[PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Rafał Miłecki]

From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>

Since early BCM5301X days we got abort handler that was removed by
commit 937b12306ea79 ("ARM: BCM5301X: remove workaround imprecise abort
fault handler"). It assumed we need to deal only with pending aborts
left by the bootloader. Unfortunately this isn't true for BCM5301X.

When probing PCI config space (device enumeration) it is expected to
have master aborts on the PCI bus. Most bridges don't forward (or they
allow disabling it) these errors onto the AXI/AMBA bus but not the
Northstar (BCM5301X) one.

iProc PCIe controller on Northstar seems to be some older one, without
a control register for errors forwarding. It means we need to workaround
this at platform level. All newer platforms are not affected by this
issue.

Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
---
 arch/arm/mach-bcm/bcm_5301x.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/arch/arm/mach-bcm/bcm_5301x.c b/arch/arm/mach-bcm/bcm_5301x.c
index c8830a2..fe067f6 100644
--- a/arch/arm/mach-bcm/bcm_5301x.c
+++ b/arch/arm/mach-bcm/bcm_5301x.c
@@ -9,14 +9,42 @@
 #include <asm/hardware/cache-l2x0.h>
 
 #include <asm/mach/arch.h>
+#include <asm/siginfo.h>
+#include <asm/signal.h>
+
+#define FSR_EXTERNAL		(1 << 12)
+#define FSR_READ		(0 << 10)
+#define FSR_IMPRECISE		0x0406
 
 static const char *const bcm5301x_dt_compat[] __initconst = {
 	"brcm,bcm4708",
 	NULL,
 };
 
+static int bcm5301x_abort_handler(unsigned long addr, unsigned int fsr,
+				  struct pt_regs *regs)
+{
+	/*
+	 * We want to ignore aborts forwarded from the PCIe bus that are
+	 * expected and shouldn't really be passed by the PCIe controller.
+	 * The biggest disadvantage is the same FSR code may be reported when
+	 * reading non-existing APB register and we shouldn't ignore that.
+	 */
+	if (fsr == (FSR_EXTERNAL | FSR_READ | FSR_IMPRECISE))
+		return 0;
+
+	return 1;
+}
+
+static void __init bcm5301x_init_early(void)
+{
+	hook_fault_code(16 + 6, bcm5301x_abort_handler, SIGBUS, BUS_OBJERR,
+			"imprecise external abort");
+}
+
 DT_MACHINE_START(BCM5301X, "BCM5301X")
 	.l2c_aux_val	= 0,
 	.l2c_aux_mask	= ~0,
 	.dt_compat	= bcm5301x_dt_compat,
+	.init_early	= bcm5301x_init_early,
 MACHINE_END
-- 
2.9.3

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

[PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Since early BCM5301X days we got abort handler that was removed by
commit 937b12306ea79 ("ARM: BCM5301X: remove workaround imprecise abort
fault handler"). It assumed we need to deal only with pending aborts
left by the bootloader. Unfortunately this isn't true for BCM5301X.

When probing PCI config space (device enumeration) it is expected to
have master aborts on the PCI bus. Most bridges don't forward (or they
allow disabling it) these errors onto the AXI/AMBA bus but not the
Northstar (BCM5301X) one.

iProc PCIe controller on Northstar seems to be some older one, without
a control register for errors forwarding. It means we need to workaround
this at platform level. All newer platforms are not affected by this
issue.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 arch/arm/mach-bcm/bcm_5301x.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/arch/arm/mach-bcm/bcm_5301x.c b/arch/arm/mach-bcm/bcm_5301x.c
index c8830a2..fe067f6 100644
--- a/arch/arm/mach-bcm/bcm_5301x.c
+++ b/arch/arm/mach-bcm/bcm_5301x.c
@@ -9,14 +9,42 @@
 #include <asm/hardware/cache-l2x0.h>
 
 #include <asm/mach/arch.h>
+#include <asm/siginfo.h>
+#include <asm/signal.h>
+
+#define FSR_EXTERNAL		(1 << 12)
+#define FSR_READ		(0 << 10)
+#define FSR_IMPRECISE		0x0406
 
 static const char *const bcm5301x_dt_compat[] __initconst = {
 	"brcm,bcm4708",
 	NULL,
 };
 
+static int bcm5301x_abort_handler(unsigned long addr, unsigned int fsr,
+				  struct pt_regs *regs)
+{
+	/*
+	 * We want to ignore aborts forwarded from the PCIe bus that are
+	 * expected and shouldn't really be passed by the PCIe controller.
+	 * The biggest disadvantage is the same FSR code may be reported when
+	 * reading non-existing APB register and we shouldn't ignore that.
+	 */
+	if (fsr == (FSR_EXTERNAL | FSR_READ | FSR_IMPRECISE))
+		return 0;
+
+	return 1;
+}
+
+static void __init bcm5301x_init_early(void)
+{
+	hook_fault_code(16 + 6, bcm5301x_abort_handler, SIGBUS, BUS_OBJERR,
+			"imprecise external abort");
+}
+
 DT_MACHINE_START(BCM5301X, "BCM5301X")
 	.l2c_aux_val	= 0,
 	.l2c_aux_mask	= ~0,
 	.dt_compat	= bcm5301x_dt_compat,
+	.init_early	= bcm5301x_init_early,
 MACHINE_END
-- 
2.9.3

[PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Rafał Miłecki]

From: Rafa? Mi?ecki <rafal@milecki.pl>

Since early BCM5301X days we got abort handler that was removed by
commit 937b12306ea79 ("ARM: BCM5301X: remove workaround imprecise abort
fault handler"). It assumed we need to deal only with pending aborts
left by the bootloader. Unfortunately this isn't true for BCM5301X.

When probing PCI config space (device enumeration) it is expected to
have master aborts on the PCI bus. Most bridges don't forward (or they
allow disabling it) these errors onto the AXI/AMBA bus but not the
Northstar (BCM5301X) one.

iProc PCIe controller on Northstar seems to be some older one, without
a control register for errors forwarding. It means we need to workaround
this at platform level. All newer platforms are not affected by this
issue.

Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl>
---
 arch/arm/mach-bcm/bcm_5301x.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/arch/arm/mach-bcm/bcm_5301x.c b/arch/arm/mach-bcm/bcm_5301x.c
index c8830a2..fe067f6 100644
--- a/arch/arm/mach-bcm/bcm_5301x.c
+++ b/arch/arm/mach-bcm/bcm_5301x.c
@@ -9,14 +9,42 @@
 #include <asm/hardware/cache-l2x0.h>
 
 #include <asm/mach/arch.h>
+#include <asm/siginfo.h>
+#include <asm/signal.h>
+
+#define FSR_EXTERNAL		(1 << 12)
+#define FSR_READ		(0 << 10)
+#define FSR_IMPRECISE		0x0406
 
 static const char *const bcm5301x_dt_compat[] __initconst = {
 	"brcm,bcm4708",
 	NULL,
 };
 
+static int bcm5301x_abort_handler(unsigned long addr, unsigned int fsr,
+				  struct pt_regs *regs)
+{
+	/*
+	 * We want to ignore aborts forwarded from the PCIe bus that are
+	 * expected and shouldn't really be passed by the PCIe controller.
+	 * The biggest disadvantage is the same FSR code may be reported when
+	 * reading non-existing APB register and we shouldn't ignore that.
+	 */
+	if (fsr == (FSR_EXTERNAL | FSR_READ | FSR_IMPRECISE))
+		return 0;
+
+	return 1;
+}
+
+static void __init bcm5301x_init_early(void)
+{
+	hook_fault_code(16 + 6, bcm5301x_abort_handler, SIGBUS, BUS_OBJERR,
+			"imprecise external abort");
+}
+
 DT_MACHINE_START(BCM5301X, "BCM5301X")
 	.l2c_aux_val	= 0,
 	.l2c_aux_mask	= ~0,
 	.dt_compat	= bcm5301x_dt_compat,
+	.init_early	= bcm5301x_init_early,
 MACHINE_END
-- 
2.9.3

Re: [PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Scott Branden]

Hi Rafal,


On 16-10-29 04:12 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> Since early BCM5301X days we got abort handler that was removed by
> commit 937b12306ea79 ("ARM: BCM5301X: remove workaround imprecise abort
> fault handler"). It assumed we need to deal only with pending aborts
> left by the bootloader. Unfortunately this isn't true for BCM5301X.
>
> When probing PCI config space (device enumeration) it is expected to
> have master aborts on the PCI bus. Most bridges don't forward (or they
> allow disabling it) these errors onto the AXI/AMBA bus but not the
> Northstar (BCM5301X) one.
Should we only add this workaround code if CONFIG_PCI is on then?

>
> iProc PCIe controller on Northstar seems to be some older one, without
> a control register for errors forwarding. It means we need to workaround
> this at platform level. All newer platforms are not affected by this
> issue.
>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  arch/arm/mach-bcm/bcm_5301x.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/arch/arm/mach-bcm/bcm_5301x.c b/arch/arm/mach-bcm/bcm_5301x.c
> index c8830a2..fe067f6 100644
> --- a/arch/arm/mach-bcm/bcm_5301x.c
> +++ b/arch/arm/mach-bcm/bcm_5301x.c
> @@ -9,14 +9,42 @@
>  #include <asm/hardware/cache-l2x0.h>
>
>  #include <asm/mach/arch.h>
> +#include <asm/siginfo.h>
> +#include <asm/signal.h>
> +
> +#define FSR_EXTERNAL		(1 << 12)
> +#define FSR_READ		(0 << 10)
> +#define FSR_IMPRECISE		0x0406
>
>  static const char *const bcm5301x_dt_compat[] __initconst = {
>  	"brcm,bcm4708",
>  	NULL,
>  };
>
> +static int bcm5301x_abort_handler(unsigned long addr, unsigned int fsr,
> +				  struct pt_regs *regs)
> +{
> +	/*
> +	 * We want to ignore aborts forwarded from the PCIe bus that are
> +	 * expected and shouldn't really be passed by the PCIe controller.
> +	 * The biggest disadvantage is the same FSR code may be reported when
> +	 * reading non-existing APB register and we shouldn't ignore that.
> +	 */
> +	if (fsr == (FSR_EXTERNAL | FSR_READ | FSR_IMPRECISE))
> +		return 0;
> +
> +	return 1;
> +}
> +
> +static void __init bcm5301x_init_early(void)
> +{
> +	hook_fault_code(16 + 6, bcm5301x_abort_handler, SIGBUS, BUS_OBJERR,
> +			"imprecise external abort");
> +}
> +
>  DT_MACHINE_START(BCM5301X, "BCM5301X")
>  	.l2c_aux_val	= 0,
>  	.l2c_aux_mask	= ~0,
>  	.dt_compat	= bcm5301x_dt_compat,
> +	.init_early	= bcm5301x_init_early,
>  MACHINE_END
>

Regards,
Scott

[PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Scott Branden]

Hi Rafal,


On 16-10-29 04:12 AM, Rafa? Mi?ecki wrote:
> From: Rafa? Mi?ecki <rafal@milecki.pl>
>
> Since early BCM5301X days we got abort handler that was removed by
> commit 937b12306ea79 ("ARM: BCM5301X: remove workaround imprecise abort
> fault handler"). It assumed we need to deal only with pending aborts
> left by the bootloader. Unfortunately this isn't true for BCM5301X.
>
> When probing PCI config space (device enumeration) it is expected to
> have master aborts on the PCI bus. Most bridges don't forward (or they
> allow disabling it) these errors onto the AXI/AMBA bus but not the
> Northstar (BCM5301X) one.
Should we only add this workaround code if CONFIG_PCI is on then?

>
> iProc PCIe controller on Northstar seems to be some older one, without
> a control register for errors forwarding. It means we need to workaround
> this at platform level. All newer platforms are not affected by this
> issue.
>
> Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl>
> ---
>  arch/arm/mach-bcm/bcm_5301x.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/arch/arm/mach-bcm/bcm_5301x.c b/arch/arm/mach-bcm/bcm_5301x.c
> index c8830a2..fe067f6 100644
> --- a/arch/arm/mach-bcm/bcm_5301x.c
> +++ b/arch/arm/mach-bcm/bcm_5301x.c
> @@ -9,14 +9,42 @@
>  #include <asm/hardware/cache-l2x0.h>
>
>  #include <asm/mach/arch.h>
> +#include <asm/siginfo.h>
> +#include <asm/signal.h>
> +
> +#define FSR_EXTERNAL		(1 << 12)
> +#define FSR_READ		(0 << 10)
> +#define FSR_IMPRECISE		0x0406
>
>  static const char *const bcm5301x_dt_compat[] __initconst = {
>  	"brcm,bcm4708",
>  	NULL,
>  };
>
> +static int bcm5301x_abort_handler(unsigned long addr, unsigned int fsr,
> +				  struct pt_regs *regs)
> +{
> +	/*
> +	 * We want to ignore aborts forwarded from the PCIe bus that are
> +	 * expected and shouldn't really be passed by the PCIe controller.
> +	 * The biggest disadvantage is the same FSR code may be reported when
> +	 * reading non-existing APB register and we shouldn't ignore that.
> +	 */
> +	if (fsr == (FSR_EXTERNAL | FSR_READ | FSR_IMPRECISE))
> +		return 0;
> +
> +	return 1;
> +}
> +
> +static void __init bcm5301x_init_early(void)
> +{
> +	hook_fault_code(16 + 6, bcm5301x_abort_handler, SIGBUS, BUS_OBJERR,
> +			"imprecise external abort");
> +}
> +
>  DT_MACHINE_START(BCM5301X, "BCM5301X")
>  	.l2c_aux_val	= 0,
>  	.l2c_aux_mask	= ~0,
>  	.dt_compat	= bcm5301x_dt_compat,
> +	.init_early	= bcm5301x_init_early,
>  MACHINE_END
>

Regards,
Scott

Re: [PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Hauke Mehrtens]

On 10/31/2016 07:08 PM, Scott Branden wrote:
> Hi Rafal,
> 
> 
> On 16-10-29 04:12 AM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>
>> Since early BCM5301X days we got abort handler that was removed by
>> commit 937b12306ea79 ("ARM: BCM5301X: remove workaround imprecise abort
>> fault handler"). It assumed we need to deal only with pending aborts
>> left by the bootloader. Unfortunately this isn't true for BCM5301X.
>>
>> When probing PCI config space (device enumeration) it is expected to
>> have master aborts on the PCI bus. Most bridges don't forward (or they
>> allow disabling it) these errors onto the AXI/AMBA bus but not the
>> Northstar (BCM5301X) one.
> Should we only add this workaround code if CONFIG_PCI is on then?

I think all the supported northstar devices have a PCIe controller. We
could add such a CONFIG_PCI check, but I do not see a big advantage.

>> iProc PCIe controller on Northstar seems to be some older one, without
>> a control register for errors forwarding. It means we need to workaround
>> this at platform level. All newer platforms are not affected by this
>> issue.
>>
>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>> ---
>>  arch/arm/mach-bcm/bcm_5301x.c | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>
>> diff --git a/arch/arm/mach-bcm/bcm_5301x.c
>> b/arch/arm/mach-bcm/bcm_5301x.c
>> index c8830a2..fe067f6 100644
>> --- a/arch/arm/mach-bcm/bcm_5301x.c
>> +++ b/arch/arm/mach-bcm/bcm_5301x.c
>> @@ -9,14 +9,42 @@
>>  #include <asm/hardware/cache-l2x0.h>
>>
>>  #include <asm/mach/arch.h>
>> +#include <asm/siginfo.h>
>> +#include <asm/signal.h>
>> +
>> +#define FSR_EXTERNAL        (1 << 12)
>> +#define FSR_READ        (0 << 10)
>> +#define FSR_IMPRECISE        0x0406
>>
>>  static const char *const bcm5301x_dt_compat[] __initconst = {
>>      "brcm,bcm4708",
>>      NULL,
>>  };
>>
>> +static int bcm5301x_abort_handler(unsigned long addr, unsigned int fsr,
>> +                  struct pt_regs *regs)
>> +{
>> +    /*
>> +     * We want to ignore aborts forwarded from the PCIe bus that are
>> +     * expected and shouldn't really be passed by the PCIe controller.
>> +     * The biggest disadvantage is the same FSR code may be reported
>> when
>> +     * reading non-existing APB register and we shouldn't ignore that.
>> +     */
>> +    if (fsr == (FSR_EXTERNAL | FSR_READ | FSR_IMPRECISE))
>> +        return 0;

How often does this happen? Would it be useful to add a log message here?

>> +
>> +    return 1;
>> +}
>> +
>> +static void __init bcm5301x_init_early(void)
>> +{
>> +    hook_fault_code(16 + 6, bcm5301x_abort_handler, SIGBUS, BUS_OBJERR,
>> +            "imprecise external abort");
>> +}
>> +
>>  DT_MACHINE_START(BCM5301X, "BCM5301X")
>>      .l2c_aux_val    = 0,
>>      .l2c_aux_mask    = ~0,
>>      .dt_compat    = bcm5301x_dt_compat,
>> +    .init_early    = bcm5301x_init_early,
>>  MACHINE_END
>>
> 
> Regards,
> Scott
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Hauke Mehrtens]

CgpPbiAxMC8zMS8yMDE2IDA3OjA4IFBNLCBTY290dCBCcmFuZGVuIHdyb3RlOgo+IEhpIFJhZmFs
LAo+IAo+IAo+IE9uIDE2LTEwLTI5IDA0OjEyIEFNLCBSYWZhxYIgTWnFgmVja2kgd3JvdGU6Cj4+
IEZyb206IFJhZmHFgiBNacWCZWNraSA8cmFmYWxAbWlsZWNraS5wbD4KPj4KPj4gU2luY2UgZWFy
bHkgQkNNNTMwMVggZGF5cyB3ZSBnb3QgYWJvcnQgaGFuZGxlciB0aGF0IHdhcyByZW1vdmVkIGJ5
Cj4+IGNvbW1pdCA5MzdiMTIzMDZlYTc5ICgiQVJNOiBCQ001MzAxWDogcmVtb3ZlIHdvcmthcm91
bmQgaW1wcmVjaXNlIGFib3J0Cj4+IGZhdWx0IGhhbmRsZXIiKS4gSXQgYXNzdW1lZCB3ZSBuZWVk
IHRvIGRlYWwgb25seSB3aXRoIHBlbmRpbmcgYWJvcnRzCj4+IGxlZnQgYnkgdGhlIGJvb3Rsb2Fk
ZXIuIFVuZm9ydHVuYXRlbHkgdGhpcyBpc24ndCB0cnVlIGZvciBCQ001MzAxWC4KPj4KPj4gV2hl
biBwcm9iaW5nIFBDSSBjb25maWcgc3BhY2UgKGRldmljZSBlbnVtZXJhdGlvbikgaXQgaXMgZXhw
ZWN0ZWQgdG8KPj4gaGF2ZSBtYXN0ZXIgYWJvcnRzIG9uIHRoZSBQQ0kgYnVzLiBNb3N0IGJyaWRn
ZXMgZG9uJ3QgZm9yd2FyZCAob3IgdGhleQo+PiBhbGxvdyBkaXNhYmxpbmcgaXQpIHRoZXNlIGVy
cm9ycyBvbnRvIHRoZSBBWEkvQU1CQSBidXMgYnV0IG5vdCB0aGUKPj4gTm9ydGhzdGFyIChCQ001
MzAxWCkgb25lLgo+IFNob3VsZCB3ZSBvbmx5IGFkZCB0aGlzIHdvcmthcm91bmQgY29kZSBpZiBD
T05GSUdfUENJIGlzIG9uIHRoZW4/CgpJIHRoaW5rIGFsbCB0aGUgc3VwcG9ydGVkIG5vcnRoc3Rh
ciBkZXZpY2VzIGhhdmUgYSBQQ0llIGNvbnRyb2xsZXIuIFdlCmNvdWxkIGFkZCBzdWNoIGEgQ09O
RklHX1BDSSBjaGVjaywgYnV0IEkgZG8gbm90IHNlZSBhIGJpZyBhZHZhbnRhZ2UuCgo+PiBpUHJv
YyBQQ0llIGNvbnRyb2xsZXIgb24gTm9ydGhzdGFyIHNlZW1zIHRvIGJlIHNvbWUgb2xkZXIgb25l
LCB3aXRob3V0Cj4+IGEgY29udHJvbCByZWdpc3RlciBmb3IgZXJyb3JzIGZvcndhcmRpbmcuIEl0
IG1lYW5zIHdlIG5lZWQgdG8gd29ya2Fyb3VuZAo+PiB0aGlzIGF0IHBsYXRmb3JtIGxldmVsLiBB
bGwgbmV3ZXIgcGxhdGZvcm1zIGFyZSBub3QgYWZmZWN0ZWQgYnkgdGhpcwo+PiBpc3N1ZS4KPj4K
Pj4gU2lnbmVkLW9mZi1ieTogUmFmYcWCIE1pxYJlY2tpIDxyYWZhbEBtaWxlY2tpLnBsPgo+PiAt
LS0KPj4gIGFyY2gvYXJtL21hY2gtYmNtL2JjbV81MzAxeC5jIHwgMjggKysrKysrKysrKysrKysr
KysrKysrKysrKysrKwo+PiAgMSBmaWxlIGNoYW5nZWQsIDI4IGluc2VydGlvbnMoKykKPj4KPj4g
ZGlmZiAtLWdpdCBhL2FyY2gvYXJtL21hY2gtYmNtL2JjbV81MzAxeC5jCj4+IGIvYXJjaC9hcm0v
bWFjaC1iY20vYmNtXzUzMDF4LmMKPj4gaW5kZXggYzg4MzBhMi4uZmUwNjdmNiAxMDA2NDQKPj4g
LS0tIGEvYXJjaC9hcm0vbWFjaC1iY20vYmNtXzUzMDF4LmMKPj4gKysrIGIvYXJjaC9hcm0vbWFj
aC1iY20vYmNtXzUzMDF4LmMKPj4gQEAgLTksMTQgKzksNDIgQEAKPj4gICNpbmNsdWRlIDxhc20v
aGFyZHdhcmUvY2FjaGUtbDJ4MC5oPgo+Pgo+PiAgI2luY2x1ZGUgPGFzbS9tYWNoL2FyY2guaD4K
Pj4gKyNpbmNsdWRlIDxhc20vc2lnaW5mby5oPgo+PiArI2luY2x1ZGUgPGFzbS9zaWduYWwuaD4K
Pj4gKwo+PiArI2RlZmluZSBGU1JfRVhURVJOQUwgICAgICAgICgxIDw8IDEyKQo+PiArI2RlZmlu
ZSBGU1JfUkVBRCAgICAgICAgKDAgPDwgMTApCj4+ICsjZGVmaW5lIEZTUl9JTVBSRUNJU0UgICAg
ICAgIDB4MDQwNgo+Pgo+PiAgc3RhdGljIGNvbnN0IGNoYXIgKmNvbnN0IGJjbTUzMDF4X2R0X2Nv
bXBhdFtdIF9faW5pdGNvbnN0ID0gewo+PiAgICAgICJicmNtLGJjbTQ3MDgiLAo+PiAgICAgIE5V
TEwsCj4+ICB9Owo+Pgo+PiArc3RhdGljIGludCBiY201MzAxeF9hYm9ydF9oYW5kbGVyKHVuc2ln
bmVkIGxvbmcgYWRkciwgdW5zaWduZWQgaW50IGZzciwKPj4gKyAgICAgICAgICAgICAgICAgIHN0
cnVjdCBwdF9yZWdzICpyZWdzKQo+PiArewo+PiArICAgIC8qCj4+ICsgICAgICogV2Ugd2FudCB0
byBpZ25vcmUgYWJvcnRzIGZvcndhcmRlZCBmcm9tIHRoZSBQQ0llIGJ1cyB0aGF0IGFyZQo+PiAr
ICAgICAqIGV4cGVjdGVkIGFuZCBzaG91bGRuJ3QgcmVhbGx5IGJlIHBhc3NlZCBieSB0aGUgUENJ
ZSBjb250cm9sbGVyLgo+PiArICAgICAqIFRoZSBiaWdnZXN0IGRpc2FkdmFudGFnZSBpcyB0aGUg
c2FtZSBGU1IgY29kZSBtYXkgYmUgcmVwb3J0ZWQKPj4gd2hlbgo+PiArICAgICAqIHJlYWRpbmcg
bm9uLWV4aXN0aW5nIEFQQiByZWdpc3RlciBhbmQgd2Ugc2hvdWxkbid0IGlnbm9yZSB0aGF0Lgo+
PiArICAgICAqLwo+PiArICAgIGlmIChmc3IgPT0gKEZTUl9FWFRFUk5BTCB8IEZTUl9SRUFEIHwg
RlNSX0lNUFJFQ0lTRSkpCj4+ICsgICAgICAgIHJldHVybiAwOwoKSG93IG9mdGVuIGRvZXMgdGhp
cyBoYXBwZW4/IFdvdWxkIGl0IGJlIHVzZWZ1bCB0byBhZGQgYSBsb2cgbWVzc2FnZSBoZXJlPwoK
Pj4gKwo+PiArICAgIHJldHVybiAxOwo+PiArfQo+PiArCj4+ICtzdGF0aWMgdm9pZCBfX2luaXQg
YmNtNTMwMXhfaW5pdF9lYXJseSh2b2lkKQo+PiArewo+PiArICAgIGhvb2tfZmF1bHRfY29kZSgx
NiArIDYsIGJjbTUzMDF4X2Fib3J0X2hhbmRsZXIsIFNJR0JVUywgQlVTX09CSkVSUiwKPj4gKyAg
ICAgICAgICAgICJpbXByZWNpc2UgZXh0ZXJuYWwgYWJvcnQiKTsKPj4gK30KPj4gKwo+PiAgRFRf
TUFDSElORV9TVEFSVChCQ001MzAxWCwgIkJDTTUzMDFYIikKPj4gICAgICAubDJjX2F1eF92YWwg
ICAgPSAwLAo+PiAgICAgIC5sMmNfYXV4X21hc2sgICAgPSB+MCwKPj4gICAgICAuZHRfY29tcGF0
ICAgID0gYmNtNTMwMXhfZHRfY29tcGF0LAo+PiArICAgIC5pbml0X2Vhcmx5ICAgID0gYmNtNTMw
MXhfaW5pdF9lYXJseSwKPj4gIE1BQ0hJTkVfRU5ECj4+Cj4gCj4gUmVnYXJkcywKPiBTY290dAoK
X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KbGludXgtYXJt
LWtlcm5lbCBtYWlsaW5nIGxpc3QKbGludXgtYXJtLWtlcm5lbEBsaXN0cy5pbmZyYWRlYWQub3Jn
Cmh0dHA6Ly9saXN0cy5pbmZyYWRlYWQub3JnL21haWxtYW4vbGlzdGluZm8vbGludXgtYXJtLWtl
cm5lbAo=

[PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Hauke Mehrtens]

On 10/31/2016 07:08 PM, Scott Branden wrote:
> Hi Rafal,
> 
> 
> On 16-10-29 04:12 AM, Rafa? Mi?ecki wrote:
>> From: Rafa? Mi?ecki <rafal@milecki.pl>
>>
>> Since early BCM5301X days we got abort handler that was removed by
>> commit 937b12306ea79 ("ARM: BCM5301X: remove workaround imprecise abort
>> fault handler"). It assumed we need to deal only with pending aborts
>> left by the bootloader. Unfortunately this isn't true for BCM5301X.
>>
>> When probing PCI config space (device enumeration) it is expected to
>> have master aborts on the PCI bus. Most bridges don't forward (or they
>> allow disabling it) these errors onto the AXI/AMBA bus but not the
>> Northstar (BCM5301X) one.
> Should we only add this workaround code if CONFIG_PCI is on then?

I think all the supported northstar devices have a PCIe controller. We
could add such a CONFIG_PCI check, but I do not see a big advantage.

>> iProc PCIe controller on Northstar seems to be some older one, without
>> a control register for errors forwarding. It means we need to workaround
>> this at platform level. All newer platforms are not affected by this
>> issue.
>>
>> Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl>
>> ---
>>  arch/arm/mach-bcm/bcm_5301x.c | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>
>> diff --git a/arch/arm/mach-bcm/bcm_5301x.c
>> b/arch/arm/mach-bcm/bcm_5301x.c
>> index c8830a2..fe067f6 100644
>> --- a/arch/arm/mach-bcm/bcm_5301x.c
>> +++ b/arch/arm/mach-bcm/bcm_5301x.c
>> @@ -9,14 +9,42 @@
>>  #include <asm/hardware/cache-l2x0.h>
>>
>>  #include <asm/mach/arch.h>
>> +#include <asm/siginfo.h>
>> +#include <asm/signal.h>
>> +
>> +#define FSR_EXTERNAL        (1 << 12)
>> +#define FSR_READ        (0 << 10)
>> +#define FSR_IMPRECISE        0x0406
>>
>>  static const char *const bcm5301x_dt_compat[] __initconst = {
>>      "brcm,bcm4708",
>>      NULL,
>>  };
>>
>> +static int bcm5301x_abort_handler(unsigned long addr, unsigned int fsr,
>> +                  struct pt_regs *regs)
>> +{
>> +    /*
>> +     * We want to ignore aborts forwarded from the PCIe bus that are
>> +     * expected and shouldn't really be passed by the PCIe controller.
>> +     * The biggest disadvantage is the same FSR code may be reported
>> when
>> +     * reading non-existing APB register and we shouldn't ignore that.
>> +     */
>> +    if (fsr == (FSR_EXTERNAL | FSR_READ | FSR_IMPRECISE))
>> +        return 0;

How often does this happen? Would it be useful to add a log message here?

>> +
>> +    return 1;
>> +}
>> +
>> +static void __init bcm5301x_init_early(void)
>> +{
>> +    hook_fault_code(16 + 6, bcm5301x_abort_handler, SIGBUS, BUS_OBJERR,
>> +            "imprecise external abort");
>> +}
>> +
>>  DT_MACHINE_START(BCM5301X, "BCM5301X")
>>      .l2c_aux_val    = 0,
>>      .l2c_aux_mask    = ~0,
>>      .dt_compat    = bcm5301x_dt_compat,
>> +    .init_early    = bcm5301x_init_early,
>>  MACHINE_END
>>
> 
> Regards,
> Scott

Re: [PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Florian Fainelli]

On 10/31/2016 01:59 PM, Hauke Mehrtens wrote:
> 
> 
> On 10/31/2016 07:08 PM, Scott Branden wrote:
>> Hi Rafal,
>>
>>
>> On 16-10-29 04:12 AM, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> Since early BCM5301X days we got abort handler that was removed by
>>> commit 937b12306ea79 ("ARM: BCM5301X: remove workaround imprecise abort
>>> fault handler"). It assumed we need to deal only with pending aborts
>>> left by the bootloader. Unfortunately this isn't true for BCM5301X.
>>>
>>> When probing PCI config space (device enumeration) it is expected to
>>> have master aborts on the PCI bus. Most bridges don't forward (or they
>>> allow disabling it) these errors onto the AXI/AMBA bus but not the
>>> Northstar (BCM5301X) one.
>> Should we only add this workaround code if CONFIG_PCI is on then?
> 
> I think all the supported northstar devices have a PCIe controller. We
> could add such a CONFIG_PCI check, but I do not see a big advantage.

Actually, I do see a couple disadvantages if we gate this with
CONFIG_PCI: if this problem shows up irrespective of your kernel
configuration, you want the error handler to clear it, not rely on
CONFIG_PCI to be enabled for the error to go away and also, without an
additional ifdef, additional compiler coverage.
-- 
Florian

[PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Florian Fainelli]

On 10/31/2016 01:59 PM, Hauke Mehrtens wrote:
> 
> 
> On 10/31/2016 07:08 PM, Scott Branden wrote:
>> Hi Rafal,
>>
>>
>> On 16-10-29 04:12 AM, Rafa? Mi?ecki wrote:
>>> From: Rafa? Mi?ecki <rafal@milecki.pl>
>>>
>>> Since early BCM5301X days we got abort handler that was removed by
>>> commit 937b12306ea79 ("ARM: BCM5301X: remove workaround imprecise abort
>>> fault handler"). It assumed we need to deal only with pending aborts
>>> left by the bootloader. Unfortunately this isn't true for BCM5301X.
>>>
>>> When probing PCI config space (device enumeration) it is expected to
>>> have master aborts on the PCI bus. Most bridges don't forward (or they
>>> allow disabling it) these errors onto the AXI/AMBA bus but not the
>>> Northstar (BCM5301X) one.
>> Should we only add this workaround code if CONFIG_PCI is on then?
> 
> I think all the supported northstar devices have a PCIe controller. We
> could add such a CONFIG_PCI check, but I do not see a big advantage.

Actually, I do see a couple disadvantages if we gate this with
CONFIG_PCI: if this problem shows up irrespective of your kernel
configuration, you want the error handler to clear it, not rely on
CONFIG_PCI to be enabled for the error to go away and also, without an
additional ifdef, additional compiler coverage.
-- 
Florian

Re: [PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Scott Branden]

On 16-10-31 02:01 PM, Florian Fainelli wrote:
> On 10/31/2016 01:59 PM, Hauke Mehrtens wrote:
>>
>>
>> On 10/31/2016 07:08 PM, Scott Branden wrote:
>>> Hi Rafal,
>>>
>>>
>>> On 16-10-29 04:12 AM, Rafał Miłecki wrote:
>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>
>>>> Since early BCM5301X days we got abort handler that was removed by
>>>> commit 937b12306ea79 ("ARM: BCM5301X: remove workaround imprecise abort
>>>> fault handler"). It assumed we need to deal only with pending aborts
>>>> left by the bootloader. Unfortunately this isn't true for BCM5301X.
>>>>
>>>> When probing PCI config space (device enumeration) it is expected to
>>>> have master aborts on the PCI bus. Most bridges don't forward (or they
>>>> allow disabling it) these errors onto the AXI/AMBA bus but not the
>>>> Northstar (BCM5301X) one.
>>> Should we only add this workaround code if CONFIG_PCI is on then?
>>
>> I think all the supported northstar devices have a PCIe controller. We
>> could add such a CONFIG_PCI check, but I do not see a big advantage.
>
> Actually, I do see a couple disadvantages if we gate this with
> CONFIG_PCI: if this problem shows up irrespective of your kernel
> configuration, you want the error handler to clear it, not rely on
> CONFIG_PCI to be enabled for the error to go away and also, without an
> additional ifdef, additional compiler coverage.
>
A problem with reintroducing this change is that all imprecise data 
aborts are ignored, not just PCI.  So if you don't actually use PCI in 
your system and want to debug other aborts you are unable to.  I don't 
know if we care about such situation.

[PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Scott Branden]

On 16-10-31 02:01 PM, Florian Fainelli wrote:
> On 10/31/2016 01:59 PM, Hauke Mehrtens wrote:
>>
>>
>> On 10/31/2016 07:08 PM, Scott Branden wrote:
>>> Hi Rafal,
>>>
>>>
>>> On 16-10-29 04:12 AM, Rafa? Mi?ecki wrote:
>>>> From: Rafa? Mi?ecki <rafal@milecki.pl>
>>>>
>>>> Since early BCM5301X days we got abort handler that was removed by
>>>> commit 937b12306ea79 ("ARM: BCM5301X: remove workaround imprecise abort
>>>> fault handler"). It assumed we need to deal only with pending aborts
>>>> left by the bootloader. Unfortunately this isn't true for BCM5301X.
>>>>
>>>> When probing PCI config space (device enumeration) it is expected to
>>>> have master aborts on the PCI bus. Most bridges don't forward (or they
>>>> allow disabling it) these errors onto the AXI/AMBA bus but not the
>>>> Northstar (BCM5301X) one.
>>> Should we only add this workaround code if CONFIG_PCI is on then?
>>
>> I think all the supported northstar devices have a PCIe controller. We
>> could add such a CONFIG_PCI check, but I do not see a big advantage.
>
> Actually, I do see a couple disadvantages if we gate this with
> CONFIG_PCI: if this problem shows up irrespective of your kernel
> configuration, you want the error handler to clear it, not rely on
> CONFIG_PCI to be enabled for the error to go away and also, without an
> additional ifdef, additional compiler coverage.
>
A problem with reintroducing this change is that all imprecise data 
aborts are ignored, not just PCI.  So if you don't actually use PCI in 
your system and want to debug other aborts you are unable to.  I don't 
know if we care about such situation.

Re: [PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Florian Fainelli]

On 10/31/2016 02:46 PM, Scott Branden wrote:
> 
> 
> On 16-10-31 02:01 PM, Florian Fainelli wrote:
>> On 10/31/2016 01:59 PM, Hauke Mehrtens wrote:
>>>
>>>
>>> On 10/31/2016 07:08 PM, Scott Branden wrote:
>>>> Hi Rafal,
>>>>
>>>>
>>>> On 16-10-29 04:12 AM, Rafał Miłecki wrote:
>>>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>>
>>>>> Since early BCM5301X days we got abort handler that was removed by
>>>>> commit 937b12306ea79 ("ARM: BCM5301X: remove workaround imprecise
>>>>> abort
>>>>> fault handler"). It assumed we need to deal only with pending aborts
>>>>> left by the bootloader. Unfortunately this isn't true for BCM5301X.
>>>>>
>>>>> When probing PCI config space (device enumeration) it is expected to
>>>>> have master aborts on the PCI bus. Most bridges don't forward (or they
>>>>> allow disabling it) these errors onto the AXI/AMBA bus but not the
>>>>> Northstar (BCM5301X) one.
>>>> Should we only add this workaround code if CONFIG_PCI is on then?
>>>
>>> I think all the supported northstar devices have a PCIe controller. We
>>> could add such a CONFIG_PCI check, but I do not see a big advantage.
>>
>> Actually, I do see a couple disadvantages if we gate this with
>> CONFIG_PCI: if this problem shows up irrespective of your kernel
>> configuration, you want the error handler to clear it, not rely on
>> CONFIG_PCI to be enabled for the error to go away and also, without an
>> additional ifdef, additional compiler coverage.
>>
> A problem with reintroducing this change is that all imprecise data
> aborts are ignored, not just PCI.  So if you don't actually use PCI in
> your system and want to debug other aborts you are unable to.  I don't
> know if we care about such situation.

Considering that any abort is pretty much fatal, the options are:

- update the freaking bootloader to a version where there are no such
aborts generated, not an option on consumer devices, unclear which
version (if any) fixes that

- fixups the aborts externally, via a boot wrapper, which is going to
take some time to develop, causes additional burden on the distributors
to provide instructions/build images on how to do it

- fixups the aborts in the kernel, irrespective of where they come from,
simple and easy

- fixups the aborts in the kernel, look where they come from, by using
some bit of magic, looking at PCIe registers and whatnot (provided that
is even possible), not too hard, but can take a while, and is error prone

I can certainly advocate that option 3 is the one that gives a working
device, and this is what matters from a distribution perspective like
LEDE/OpenWrt.
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Florian Fainelli]

On 10/31/2016 02:46 PM, Scott Branden wrote:
> 
> 
> On 16-10-31 02:01 PM, Florian Fainelli wrote:
>> On 10/31/2016 01:59 PM, Hauke Mehrtens wrote:
>>>
>>>
>>> On 10/31/2016 07:08 PM, Scott Branden wrote:
>>>> Hi Rafal,
>>>>
>>>>
>>>> On 16-10-29 04:12 AM, Rafał Miłecki wrote:
>>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>>
>>>>> Since early BCM5301X days we got abort handler that was removed by
>>>>> commit 937b12306ea79 ("ARM: BCM5301X: remove workaround imprecise
>>>>> abort
>>>>> fault handler"). It assumed we need to deal only with pending aborts
>>>>> left by the bootloader. Unfortunately this isn't true for BCM5301X.
>>>>>
>>>>> When probing PCI config space (device enumeration) it is expected to
>>>>> have master aborts on the PCI bus. Most bridges don't forward (or they
>>>>> allow disabling it) these errors onto the AXI/AMBA bus but not the
>>>>> Northstar (BCM5301X) one.
>>>> Should we only add this workaround code if CONFIG_PCI is on then?
>>>
>>> I think all the supported northstar devices have a PCIe controller. We
>>> could add such a CONFIG_PCI check, but I do not see a big advantage.
>>
>> Actually, I do see a couple disadvantages if we gate this with
>> CONFIG_PCI: if this problem shows up irrespective of your kernel
>> configuration, you want the error handler to clear it, not rely on
>> CONFIG_PCI to be enabled for the error to go away and also, without an
>> additional ifdef, additional compiler coverage.
>>
> A problem with reintroducing this change is that all imprecise data
> aborts are ignored, not just PCI.  So if you don't actually use PCI in
> your system and want to debug other aborts you are unable to.  I don't
> know if we care about such situation.

Considering that any abort is pretty much fatal, the options are:

- update the freaking bootloader to a version where there are no such
aborts generated, not an option on consumer devices, unclear which
version (if any) fixes that

- fixups the aborts externally, via a boot wrapper, which is going to
take some time to develop, causes additional burden on the distributors
to provide instructions/build images on how to do it

- fixups the aborts in the kernel, irrespective of where they come from,
simple and easy

- fixups the aborts in the kernel, look where they come from, by using
some bit of magic, looking at PCIe registers and whatnot (provided that
is even possible), not too hard, but can take a while, and is error prone

I can certainly advocate that option 3 is the one that gives a working
device, and this is what matters from a distribution perspective like
LEDE/OpenWrt.
-- 
Florian

[PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Florian Fainelli]

On 10/31/2016 02:46 PM, Scott Branden wrote:
> 
> 
> On 16-10-31 02:01 PM, Florian Fainelli wrote:
>> On 10/31/2016 01:59 PM, Hauke Mehrtens wrote:
>>>
>>>
>>> On 10/31/2016 07:08 PM, Scott Branden wrote:
>>>> Hi Rafal,
>>>>
>>>>
>>>> On 16-10-29 04:12 AM, Rafa? Mi?ecki wrote:
>>>>> From: Rafa? Mi?ecki <rafal@milecki.pl>
>>>>>
>>>>> Since early BCM5301X days we got abort handler that was removed by
>>>>> commit 937b12306ea79 ("ARM: BCM5301X: remove workaround imprecise
>>>>> abort
>>>>> fault handler"). It assumed we need to deal only with pending aborts
>>>>> left by the bootloader. Unfortunately this isn't true for BCM5301X.
>>>>>
>>>>> When probing PCI config space (device enumeration) it is expected to
>>>>> have master aborts on the PCI bus. Most bridges don't forward (or they
>>>>> allow disabling it) these errors onto the AXI/AMBA bus but not the
>>>>> Northstar (BCM5301X) one.
>>>> Should we only add this workaround code if CONFIG_PCI is on then?
>>>
>>> I think all the supported northstar devices have a PCIe controller. We
>>> could add such a CONFIG_PCI check, but I do not see a big advantage.
>>
>> Actually, I do see a couple disadvantages if we gate this with
>> CONFIG_PCI: if this problem shows up irrespective of your kernel
>> configuration, you want the error handler to clear it, not rely on
>> CONFIG_PCI to be enabled for the error to go away and also, without an
>> additional ifdef, additional compiler coverage.
>>
> A problem with reintroducing this change is that all imprecise data
> aborts are ignored, not just PCI.  So if you don't actually use PCI in
> your system and want to debug other aborts you are unable to.  I don't
> know if we care about such situation.

Considering that any abort is pretty much fatal, the options are:

- update the freaking bootloader to a version where there are no such
aborts generated, not an option on consumer devices, unclear which
version (if any) fixes that

- fixups the aborts externally, via a boot wrapper, which is going to
take some time to develop, causes additional burden on the distributors
to provide instructions/build images on how to do it

- fixups the aborts in the kernel, irrespective of where they come from,
simple and easy

- fixups the aborts in the kernel, look where they come from, by using
some bit of magic, looking at PCIe registers and whatnot (provided that
is even possible), not too hard, but can take a while, and is error prone

I can certainly advocate that option 3 is the one that gives a working
device, and this is what matters from a distribution perspective like
LEDE/OpenWrt.
-- 
Florian

Re: [PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Rafał Miłecki]

On 31 October 2016 at 22:56, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 10/31/2016 02:46 PM, Scott Branden wrote:
>> A problem with reintroducing this change is that all imprecise data
>> aborts are ignored, not just PCI.  So if you don't actually use PCI in
>> your system and want to debug other aborts you are unable to.  I don't
>> know if we care about such situation.
>
> Considering that any abort is pretty much fatal, the options are:
>
> - update the freaking bootloader to a version where there are no such
> aborts generated, not an option on consumer devices, unclear which
> version (if any) fixes that
>
> - fixups the aborts externally, via a boot wrapper, which is going to
> take some time to develop, causes additional burden on the distributors
> to provide instructions/build images on how to do it

In practice updating bootloader is not possible (as you noticed) but I
don't think it's even possible to fix this problem at bootloader /
extra loader level. If this was a matter of hardware setup, we could
handle it in kernel as well I believe. Ray actually verified it's
controller limitation on Northstar platform.
It sounds a bit like you're thinking about MMU and Dcache Northstar
problem when writing that e-mail.


> - fixups the aborts in the kernel, irrespective of where they come from,
> simple and easy
>
> - fixups the aborts in the kernel, look where they come from, by using
> some bit of magic, looking at PCIe registers and whatnot (provided that
> is even possible), not too hard, but can take a while, and is error prone
>
> I can certainly advocate that option 3 is the one that gives a working
> device, and this is what matters from a distribution perspective like
> LEDE/OpenWrt.

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

Re: [PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Rafał Miłecki]

On 31 October 2016 at 22:56, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 10/31/2016 02:46 PM, Scott Branden wrote:
>> A problem with reintroducing this change is that all imprecise data
>> aborts are ignored, not just PCI.  So if you don't actually use PCI in
>> your system and want to debug other aborts you are unable to.  I don't
>> know if we care about such situation.
>
> Considering that any abort is pretty much fatal, the options are:
>
> - update the freaking bootloader to a version where there are no such
> aborts generated, not an option on consumer devices, unclear which
> version (if any) fixes that
>
> - fixups the aborts externally, via a boot wrapper, which is going to
> take some time to develop, causes additional burden on the distributors
> to provide instructions/build images on how to do it

In practice updating bootloader is not possible (as you noticed) but I
don't think it's even possible to fix this problem at bootloader /
extra loader level. If this was a matter of hardware setup, we could
handle it in kernel as well I believe. Ray actually verified it's
controller limitation on Northstar platform.
It sounds a bit like you're thinking about MMU and Dcache Northstar
problem when writing that e-mail.


> - fixups the aborts in the kernel, irrespective of where they come from,
> simple and easy
>
> - fixups the aborts in the kernel, look where they come from, by using
> some bit of magic, looking at PCIe registers and whatnot (provided that
> is even possible), not too hard, but can take a while, and is error prone
>
> I can certainly advocate that option 3 is the one that gives a working
> device, and this is what matters from a distribution perspective like
> LEDE/OpenWrt.

--=20
Rafa=C5=82

[PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Rafał Miłecki]

On 31 October 2016 at 22:56, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 10/31/2016 02:46 PM, Scott Branden wrote:
>> A problem with reintroducing this change is that all imprecise data
>> aborts are ignored, not just PCI.  So if you don't actually use PCI in
>> your system and want to debug other aborts you are unable to.  I don't
>> know if we care about such situation.
>
> Considering that any abort is pretty much fatal, the options are:
>
> - update the freaking bootloader to a version where there are no such
> aborts generated, not an option on consumer devices, unclear which
> version (if any) fixes that
>
> - fixups the aborts externally, via a boot wrapper, which is going to
> take some time to develop, causes additional burden on the distributors
> to provide instructions/build images on how to do it

In practice updating bootloader is not possible (as you noticed) but I
don't think it's even possible to fix this problem at bootloader /
extra loader level. If this was a matter of hardware setup, we could
handle it in kernel as well I believe. Ray actually verified it's
controller limitation on Northstar platform.
It sounds a bit like you're thinking about MMU and Dcache Northstar
problem when writing that e-mail.


> - fixups the aborts in the kernel, irrespective of where they come from,
> simple and easy
>
> - fixups the aborts in the kernel, look where they come from, by using
> some bit of magic, looking at PCIe registers and whatnot (provided that
> is even possible), not too hard, but can take a while, and is error prone
>
> I can certainly advocate that option 3 is the one that gives a working
> device, and this is what matters from a distribution perspective like
> LEDE/OpenWrt.

-- 
Rafa?

Re: [PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Scott Branden]

On 16-10-31 02:56 PM, Florian Fainelli wrote:
> On 10/31/2016 02:46 PM, Scott Branden wrote:
>>
>>
>> On 16-10-31 02:01 PM, Florian Fainelli wrote:
>>> On 10/31/2016 01:59 PM, Hauke Mehrtens wrote:
>>>>
>>>>
>>>> On 10/31/2016 07:08 PM, Scott Branden wrote:
>>>>> Hi Rafal,
>>>>>
>>>>>
>>>>> On 16-10-29 04:12 AM, Rafał Miłecki wrote:
>>>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>>>
>>>>>> Since early BCM5301X days we got abort handler that was removed by
>>>>>> commit 937b12306ea79 ("ARM: BCM5301X: remove workaround imprecise
>>>>>> abort
>>>>>> fault handler"). It assumed we need to deal only with pending aborts
>>>>>> left by the bootloader. Unfortunately this isn't true for BCM5301X.
>>>>>>
>>>>>> When probing PCI config space (device enumeration) it is expected to
>>>>>> have master aborts on the PCI bus. Most bridges don't forward (or they
>>>>>> allow disabling it) these errors onto the AXI/AMBA bus but not the
>>>>>> Northstar (BCM5301X) one.
>>>>> Should we only add this workaround code if CONFIG_PCI is on then?
>>>>
>>>> I think all the supported northstar devices have a PCIe controller. We
>>>> could add such a CONFIG_PCI check, but I do not see a big advantage.
>>>
>>> Actually, I do see a couple disadvantages if we gate this with
>>> CONFIG_PCI: if this problem shows up irrespective of your kernel
>>> configuration, you want the error handler to clear it, not rely on
>>> CONFIG_PCI to be enabled for the error to go away and also, without an
>>> additional ifdef, additional compiler coverage.
>>>
>> A problem with reintroducing this change is that all imprecise data
>> aborts are ignored, not just PCI.  So if you don't actually use PCI in
>> your system and want to debug other aborts you are unable to.  I don't
>> know if we care about such situation.
>
> Considering that any abort is pretty much fatal, the options are:
>
> - update the freaking bootloader to a version where there are no such
> aborts generated, not an option on consumer devices, unclear which
> version (if any) fixes that
>
> - fixups the aborts externally, via a boot wrapper, which is going to
> take some time to develop, causes additional burden on the distributors
> to provide instructions/build images on how to do it
I think the abort is already fixed in the kernel now on boot so option 1 
and 2 were not needed - and abort handler was removed as detailed by 
Rafal in the commit.  Only outstanding issue is now this new PCI 
enumeration issue.
>
> - fixups the aborts in the kernel, irrespective of where they come from,
> simple and easy
>
> - fixups the aborts in the kernel, look where they come from, by using
> some bit of magic, looking at PCIe registers and whatnot (provided that
> is even possible), not too hard, but can take a while, and is error prone
Option 4 sounds like the proper solution - check the range the abort is 
due to the PCI device enumeration and only ignore those aborts.
>
> I can certainly advocate that option 3 is the one that gives a working
> device, and this is what matters from a distribution perspective like
> LEDE/OpenWrt.
>
Since I don't use BCM5301x option 3 is fine by me.

[PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Scott Branden]

On 16-10-31 02:56 PM, Florian Fainelli wrote:
> On 10/31/2016 02:46 PM, Scott Branden wrote:
>>
>>
>> On 16-10-31 02:01 PM, Florian Fainelli wrote:
>>> On 10/31/2016 01:59 PM, Hauke Mehrtens wrote:
>>>>
>>>>
>>>> On 10/31/2016 07:08 PM, Scott Branden wrote:
>>>>> Hi Rafal,
>>>>>
>>>>>
>>>>> On 16-10-29 04:12 AM, Rafa? Mi?ecki wrote:
>>>>>> From: Rafa? Mi?ecki <rafal@milecki.pl>
>>>>>>
>>>>>> Since early BCM5301X days we got abort handler that was removed by
>>>>>> commit 937b12306ea79 ("ARM: BCM5301X: remove workaround imprecise
>>>>>> abort
>>>>>> fault handler"). It assumed we need to deal only with pending aborts
>>>>>> left by the bootloader. Unfortunately this isn't true for BCM5301X.
>>>>>>
>>>>>> When probing PCI config space (device enumeration) it is expected to
>>>>>> have master aborts on the PCI bus. Most bridges don't forward (or they
>>>>>> allow disabling it) these errors onto the AXI/AMBA bus but not the
>>>>>> Northstar (BCM5301X) one.
>>>>> Should we only add this workaround code if CONFIG_PCI is on then?
>>>>
>>>> I think all the supported northstar devices have a PCIe controller. We
>>>> could add such a CONFIG_PCI check, but I do not see a big advantage.
>>>
>>> Actually, I do see a couple disadvantages if we gate this with
>>> CONFIG_PCI: if this problem shows up irrespective of your kernel
>>> configuration, you want the error handler to clear it, not rely on
>>> CONFIG_PCI to be enabled for the error to go away and also, without an
>>> additional ifdef, additional compiler coverage.
>>>
>> A problem with reintroducing this change is that all imprecise data
>> aborts are ignored, not just PCI.  So if you don't actually use PCI in
>> your system and want to debug other aborts you are unable to.  I don't
>> know if we care about such situation.
>
> Considering that any abort is pretty much fatal, the options are:
>
> - update the freaking bootloader to a version where there are no such
> aborts generated, not an option on consumer devices, unclear which
> version (if any) fixes that
>
> - fixups the aborts externally, via a boot wrapper, which is going to
> take some time to develop, causes additional burden on the distributors
> to provide instructions/build images on how to do it
I think the abort is already fixed in the kernel now on boot so option 1 
and 2 were not needed - and abort handler was removed as detailed by 
Rafal in the commit.  Only outstanding issue is now this new PCI 
enumeration issue.
>
> - fixups the aborts in the kernel, irrespective of where they come from,
> simple and easy
>
> - fixups the aborts in the kernel, look where they come from, by using
> some bit of magic, looking at PCIe registers and whatnot (provided that
> is even possible), not too hard, but can take a while, and is error prone
Option 4 sounds like the proper solution - check the range the abort is 
due to the PCI device enumeration and only ignore those aborts.
>
> I can certainly advocate that option 3 is the one that gives a working
> device, and this is what matters from a distribution perspective like
> LEDE/OpenWrt.
>
Since I don't use BCM5301x option 3 is fine by me.

Re: [PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Rafał Miłecki]

On 31 October 2016 at 23:05, Scott Branden <scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
> On 16-10-31 02:56 PM, Florian Fainelli wrote:
>> - fixups the aborts in the kernel, look where they come from, by using
>> some bit of magic, looking at PCIe registers and whatnot (provided that
>> is even possible), not too hard, but can take a while, and is error prone
>
> Option 4 sounds like the proper solution - check the range the abort is due
> to the PCI device enumeration and only ignore those aborts.

This was already suggested by Arnd:
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/422873.html
and Ray was checking some internal datasheets, but I'm afraid he never
found info on checking if error was caused by PCIe controller.

Maybe we could think about some BCM5301X API (extra symbol exported by
arch code) for starting ignoring aborts and stopping that. We could
ignore aborts during scanning only but honestly it sounds like a bit
hacky solution to me.


>> I can certainly advocate that option 3 is the one that gives a working
>> device, and this is what matters from a distribution perspective like
>> LEDE/OpenWrt.
>>
> Since I don't use BCM5301x option 3 is fine by me.

So for it seems like the best solution to me, but I'm open for changes
if someone points another that is clean & better one.

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

Re: [PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Rafał Miłecki]

On 31 October 2016 at 23:05, Scott Branden <scott.branden@broadcom.com> wro=
te:
> On 16-10-31 02:56 PM, Florian Fainelli wrote:
>> - fixups the aborts in the kernel, look where they come from, by using
>> some bit of magic, looking at PCIe registers and whatnot (provided that
>> is even possible), not too hard, but can take a while, and is error pron=
e
>
> Option 4 sounds like the proper solution - check the range the abort is d=
ue
> to the PCI device enumeration and only ignore those aborts.

This was already suggested by Arnd:
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/422873.htm=
l
and Ray was checking some internal datasheets, but I'm afraid he never
found info on checking if error was caused by PCIe controller.

Maybe we could think about some BCM5301X API (extra symbol exported by
arch code) for starting ignoring aborts and stopping that. We could
ignore aborts during scanning only but honestly it sounds like a bit
hacky solution to me.


>> I can certainly advocate that option 3 is the one that gives a working
>> device, and this is what matters from a distribution perspective like
>> LEDE/OpenWrt.
>>
> Since I don't use BCM5301x option 3 is fine by me.

So for it seems like the best solution to me, but I'm open for changes
if someone points another that is clean & better one.

--=20
Rafa=C5=82

[PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Rafał Miłecki]

On 31 October 2016 at 23:05, Scott Branden <scott.branden@broadcom.com> wrote:
> On 16-10-31 02:56 PM, Florian Fainelli wrote:
>> - fixups the aborts in the kernel, look where they come from, by using
>> some bit of magic, looking at PCIe registers and whatnot (provided that
>> is even possible), not too hard, but can take a while, and is error prone
>
> Option 4 sounds like the proper solution - check the range the abort is due
> to the PCI device enumeration and only ignore those aborts.

This was already suggested by Arnd:
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/422873.html
and Ray was checking some internal datasheets, but I'm afraid he never
found info on checking if error was caused by PCIe controller.

Maybe we could think about some BCM5301X API (extra symbol exported by
arch code) for starting ignoring aborts and stopping that. We could
ignore aborts during scanning only but honestly it sounds like a bit
hacky solution to me.


>> I can certainly advocate that option 3 is the one that gives a working
>> device, and this is what matters from a distribution perspective like
>> LEDE/OpenWrt.
>>
> Since I don't use BCM5301x option 3 is fine by me.

So for it seems like the best solution to me, but I'm open for changes
if someone points another that is clean & better one.

-- 
Rafa?

Re: [PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Ray Jui]

Hi Rafal,

On 10/31/2016 3:16 PM, Rafał Miłecki wrote:
> On 31 October 2016 at 23:05, Scott Branden <scott.branden@broadcom.com> wrote:
>> On 16-10-31 02:56 PM, Florian Fainelli wrote:
>>> - fixups the aborts in the kernel, look where they come from, by using
>>> some bit of magic, looking at PCIe registers and whatnot (provided that
>>> is even possible), not too hard, but can take a while, and is error prone
>>
>> Option 4 sounds like the proper solution - check the range the abort is due
>> to the PCI device enumeration and only ignore those aborts.
> 
> This was already suggested by Arnd:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/422873.html
> and Ray was checking some internal datasheets, but I'm afraid he never
> found info on checking if error was caused by PCIe controller.
> 

Correct. Based on response from our ASIC team, on Northstar, 1) we
cannot distinguish between various external imprecise aborts; 2) nor can
we use the internal PCIe controller register to disable unsupported
request from being forwarded as APB bus error (and therefore causes the
abort).

All subsequent iProc SoCs can disable this error forwarding from the
iProc PCIe controller.

> Maybe we could think about some BCM5301X API (extra symbol exported by
> arch code) for starting ignoring aborts and stopping that. We could
> ignore aborts during scanning only but honestly it sounds like a bit
> hacky solution to me.
> 
> 

Correct, one possible solution is to only ignoring the aborts during
condiguration space register access of an endpoint device. Below is the
logic how I only disable the APB bus error forwarding during these
accesses (for all other iProc SoCs):

+/**
+ * APB error forwarding can be disabled during access of configuration
+ * registers of the endpoint device, to prevent unsupported requests
+ * (typically seen during enumeration with multi-function devices) from
+ * triggering a system exception
+ */
+static inline void iproc_pcie_apb_err_disable(struct pci_bus *bus,
+					      bool disable)
+{
+	struct iproc_pcie *pcie = iproc_data(bus);
+	u32 val;
+
+	if (bus->number && pcie->has_apb_err_disable) {
+		val = iproc_pcie_read_reg(pcie, IPROC_PCIE_APB_ERR_EN);
+		if (disable)
+			val &= ~APB_ERR_EN;
+		else
+			val |= APB_ERR_EN;
+		iproc_pcie_write_reg(pcie, IPROC_PCIE_APB_ERR_EN, val);
+	}
+}
+
[...]
+static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int
devfn,
+				    int where, int size, u32 *val)
+{
+	int ret;
+
+	iproc_pcie_apb_err_disable(bus, true);
+	ret = pci_generic_config_read32(bus, devfn, where, size, val);
+	iproc_pcie_apb_err_disable(bus, false);
+
+	return ret;
+}
+
+static int iproc_pcie_config_write32(struct pci_bus *bus, unsigned int
devfn,
+				     int where, int size, u32 val)
+{
+	int ret;
+
+	iproc_pcie_apb_err_disable(bus, true);
+	ret = pci_generic_config_write32(bus, devfn, where, size, val);
+	iproc_pcie_apb_err_disable(bus, false);
+
+	return ret;
+}
+
 static struct pci_ops iproc_pcie_ops = {
 	.map_bus = iproc_pcie_map_cfg_bus,
-	.read = pci_generic_config_read32,
-	.write = pci_generic_config_write32,
+	.read = iproc_pcie_config_read32,
+	.write = iproc_pcie_config_write32,
 };

>>> I can certainly advocate that option 3 is the one that gives a working
>>> device, and this is what matters from a distribution perspective like
>>> LEDE/OpenWrt.
>>>
>> Since I don't use BCM5301x option 3 is fine by me.
> 
> So for it seems like the best solution to me, but I'm open for changes
> if someone points another that is clean & better one.
> 

I agree with that option 3 is probably the best solution for now, from a
distribution's perspective.

Thanks,

Ray

[PATCH] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

[Ray Jui]

Hi Rafal,

On 10/31/2016 3:16 PM, Rafa? Mi?ecki wrote:
> On 31 October 2016 at 23:05, Scott Branden <scott.branden@broadcom.com> wrote:
>> On 16-10-31 02:56 PM, Florian Fainelli wrote:
>>> - fixups the aborts in the kernel, look where they come from, by using
>>> some bit of magic, looking at PCIe registers and whatnot (provided that
>>> is even possible), not too hard, but can take a while, and is error prone
>>
>> Option 4 sounds like the proper solution - check the range the abort is due
>> to the PCI device enumeration and only ignore those aborts.
> 
> This was already suggested by Arnd:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/422873.html
> and Ray was checking some internal datasheets, but I'm afraid he never
> found info on checking if error was caused by PCIe controller.
> 

Correct. Based on response from our ASIC team, on Northstar, 1) we
cannot distinguish between various external imprecise aborts; 2) nor can
we use the internal PCIe controller register to disable unsupported
request from being forwarded as APB bus error (and therefore causes the
abort).

All subsequent iProc SoCs can disable this error forwarding from the
iProc PCIe controller.

> Maybe we could think about some BCM5301X API (extra symbol exported by
> arch code) for starting ignoring aborts and stopping that. We could
> ignore aborts during scanning only but honestly it sounds like a bit
> hacky solution to me.
> 
> 

Correct, one possible solution is to only ignoring the aborts during
condiguration space register access of an endpoint device. Below is the
logic how I only disable the APB bus error forwarding during these
accesses (for all other iProc SoCs):

+/**
+ * APB error forwarding can be disabled during access of configuration
+ * registers of the endpoint device, to prevent unsupported requests
+ * (typically seen during enumeration with multi-function devices) from
+ * triggering a system exception
+ */
+static inline void iproc_pcie_apb_err_disable(struct pci_bus *bus,
+					      bool disable)
+{
+	struct iproc_pcie *pcie = iproc_data(bus);
+	u32 val;
+
+	if (bus->number && pcie->has_apb_err_disable) {
+		val = iproc_pcie_read_reg(pcie, IPROC_PCIE_APB_ERR_EN);
+		if (disable)
+			val &= ~APB_ERR_EN;
+		else
+			val |= APB_ERR_EN;
+		iproc_pcie_write_reg(pcie, IPROC_PCIE_APB_ERR_EN, val);
+	}
+}
+
[...]
+static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int
devfn,
+				    int where, int size, u32 *val)
+{
+	int ret;
+
+	iproc_pcie_apb_err_disable(bus, true);
+	ret = pci_generic_config_read32(bus, devfn, where, size, val);
+	iproc_pcie_apb_err_disable(bus, false);
+
+	return ret;
+}
+
+static int iproc_pcie_config_write32(struct pci_bus *bus, unsigned int
devfn,
+				     int where, int size, u32 val)
+{
+	int ret;
+
+	iproc_pcie_apb_err_disable(bus, true);
+	ret = pci_generic_config_write32(bus, devfn, where, size, val);
+	iproc_pcie_apb_err_disable(bus, false);
+
+	return ret;
+}
+
 static struct pci_ops iproc_pcie_ops = {
 	.map_bus = iproc_pcie_map_cfg_bus,
-	.read = pci_generic_config_read32,
-	.write = pci_generic_config_write32,
+	.read = iproc_pcie_config_read32,
+	.write = iproc_pcie_config_write32,
 };

>>> I can certainly advocate that option 3 is the one that gives a working
>>> device, and this is what matters from a distribution perspective like
>>> LEDE/OpenWrt.
>>>
>> Since I don't use BCM5301x option 3 is fine by me.
> 
> So for it seems like the best solution to me, but I'm open for changes
> if someone points another that is clean & better one.
> 

I agree with that option 3 is probably the best solution for now, from a
distribution's perspective.

Thanks,

Ray