PCI: iproc: Support DT property for ignoring aborts when probing

[auto build test ERROR on pci/next]
[also build test ERROR on v4.6-rc2 next-20160408]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Rafa-Mi-ecki/PCI-iproc-Support-DT-property-for-ignoring-aborts-when-probing/20160410-055241
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: arm-allmodconfig (attached as .config)

reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm

All errors (new ones prefixed by >>):

Quoted 1 line

Quoted 1 line

ERROR: "hook_fault_code" [drivers/pci/host/pcie-iproc.ko] undefined!

Some devices (e.g. Northstar ones) may have bridges that forward harmless errors to the ARM core. In such case we need an option to add a handler ignoring them.

Signed-off-by Rafa? Mi?ecki <[email protected]>

---

+- brcm,pcie-hook-abort-handler: During PCI bus probing (device enumeration)
+ there can be errors that are expected and harmless. Unfortunately some bridges
+ can't be configured to ignore them and they forward them to the ARM core

(quoted diff) +2 -0

+  triggering die().
+  This property should be set in such case, it will make driver add

its own
+ handler ignoring such errors.

Quoted 15 lines · Rafa? Mi?ecki

Some devices (e.g. Northstar ones) may have bridges that forward harmless errors to the ARM core. In such case we need an option to add a handler ignoring them.

Signed-off-by Rafa? Mi?ecki <[email protected]>

---

+- brcm,pcie-hook-abort-handler: During PCI bus probing (device enumeration)
+ there can be errors that are expected and harmless. Unfortunately some bridges
+ can't be configured to ignore them and they forward them to the ARM core

(quoted diff) +2 -0

+  triggering die().
+  This property should be set in such case, it will make driver add

Quoted 2 lines

its own
+ handler ignoring such errors.

The property is named after the function that allows you to catch abort handlers, whereas you should be describing the HW here. Something like brcm,bridge-error-forward or a better name even would be preferable.

drivers/pci/host/pcie-iproc.c static int iproc_pcie_map_ranges(struct iproc_pcie *pcie, · line 443

Quoted 9 lines

(quoted diff) +2 -0

+#ifdef CONFIG_ARM
+static int iproc_pcie_abort_handler(unsigned long addr, unsigned int

fsr,

drivers/pci/host/pcie-iproc.c +6 -0

+				    struct pt_regs *regs)
+{
+	if (fsr == 0x1406)
+		return 0;
+
+	return 1;

As you later noted this prevents this driver from being a module now. Since the expectation is that either a fixed bootloader or a platform should enot produce these data aborts, or allow them to be ignored, why not just put this code back where it belongs in the machine specific file which kills many birds with the same stone:

- code is ways built-in, and hook_fault_code is installed prior to PCIe loading (function is marked with __init)
- platforms which do not need that, just do not install it for that specific code
- it is clear which platforms need it and which do not, yet the driver remains agnostic

NB: there could be other platforms some day needing that which also propagate the error differently, forcing you to add more and more of these codes in the PCIe driver.

Quoted 25 lines · Florian Fainelli

Quoted 15 lines · Rafa? Mi?ecki

Some devices (e.g. Northstar ones) may have bridges that forward harmless errors to the ARM core. In such case we need an option to add a handler ignoring them.

Signed-off-by Rafa? Mi?ecki <[email protected]>

---

+- brcm,pcie-hook-abort-handler: During PCI bus probing (device enumeration)
+ there can be errors that are expected and harmless. Unfortunately some bridges
+ can't be configured to ignore them and they forward them to the ARM core

(quoted diff) +2 -0

+  triggering die().
+  This property should be set in such case, it will make driver add

Quoted 2 lines

its own
+ handler ignoring such errors.

The property is named after the function that allows you to catch abort handlers, whereas you should be describing the HW here. Something like brcm,bridge-error-forward or a better name even would be preferable.

Quoted 16 lines

drivers/pci/host/pcie-iproc.c static int iproc_pcie_map_ranges(struct iproc_pcie *pcie, · line 443

Quoted 9 lines

(quoted diff) +2 -0

+#ifdef CONFIG_ARM
+static int iproc_pcie_abort_handler(unsigned long addr, unsigned int

fsr,

drivers/pci/host/pcie-iproc.c +6 -0

+                    struct pt_regs *regs)
+{
+    if (fsr == 0x1406)
+        return 0;
+
+    return 1;

As you later noted this prevents this driver from being a module now. Since the expectation is that either a fixed bootloader or a platform should enot produce these data aborts, or allow them to be ignored, why not just put this code back where it belongs in the machine specific file which kills many birds with the same stone:

I assume the module compile issue can be simply fixed by exporting symbol of "hook_fault_code"?

I believe ARM64 based NS2 that uses the same iProc PCIe driver might also need something like this (I'm still waiting for Jon Mason to give me some more information on the NS2 errors that he saw, which is likely related to this). I assume there will be something similar to the ARM specific "hook_fault_code" for ARM64, but then ARM64 does not have any "mach" specific directory. Where can this type of hook be installed for ARM64 based platforms if not in the PCIe driver?

Quoted 7 lines

- code is ways built-in, and hook_fault_code is installed prior to PCIe loading (function is marked with __init)
- platforms which do not need that, just do not install it for that specific code
- it is clear which platforms need it and which do not, yet the driver remains agnostic

It's actually unclear to me which iProc platforms need this.
- We know NS needs it
- Someone mentioned NSP does not need this? But where does the information come from? Do we have similar PCIe connection configuration on NSP as NS?
- I cannot confirm whether Cygnus needs this or not since we do not have similar board configurations on Cygnus (on Cygnus each host is always connected to a single device). I can check with the ASIC team, but that will be a very lengthy process and I'm not sure when I can get the answer we need (and yes I do work at Broadcom, :))
- Jon Mason reported similar abort issues on NS2 when an externally connected, multi function device is used. I'm still waiting for him to send me the error log and see if it's the same type of data abort.

Quoted 6 lines

NB: there could be other platforms some day needing that which also propagate the error differently, forcing you to add more and more of these codes in the PCIe driver.

Yes, at this point, we are unsure whether the same or different code is used among different platforms.

Thanks,

Ray

Quoted 45 lines · Ray Jui

Quoted 41 lines · Florian Fainelli

Quoted 15 lines · Rafa? Mi?ecki

Some devices (e.g. Northstar ones) may have bridges that forward harmless errors to the ARM core. In such case we need an option to add a handler ignoring them.

Signed-off-by Rafa? Mi?ecki <[email protected]>

---

+- brcm,pcie-hook-abort-handler: During PCI bus probing (device enumeration)
+ there can be errors that are expected and harmless. Unfortunately some bridges
+ can't be configured to ignore them and they forward them to the ARM core

(quoted diff) +2 -0

+  triggering die().
+  This property should be set in such case, it will make driver add

Quoted 2 lines

its own
+ handler ignoring such errors.

The property is named after the function that allows you to catch abort handlers, whereas you should be describing the HW here. Something like brcm,bridge-error-forward or a better name even would be preferable.

drivers/pci/host/pcie-iproc.c static int iproc_pcie_map_ranges(struct iproc_pcie *pcie, · line 443

Quoted 9 lines

(quoted diff) +2 -0

+#ifdef CONFIG_ARM
+static int iproc_pcie_abort_handler(unsigned long addr, unsigned int

fsr,

drivers/pci/host/pcie-iproc.c +6 -0

+                    struct pt_regs *regs)
+{
+    if (fsr == 0x1406)
+        return 0;
+
+    return 1;

As you later noted this prevents this driver from being a module now. Since the expectation is that either a fixed bootloader or a platform should enot produce these data aborts, or allow them to be ignored, why not just put this code back where it belongs in the machine specific file which kills many birds with the same stone:

Quoted 2 lines

I assume the module compile issue can be simply fixed by exporting symbol of "hook_fault_code"?

I do not think it is desireable for this symbol to be exported in the first place, also last I looked, this was a one time registration thing, you cannot undo the hook you installed, but everything can be fixed.

Quoted 8 lines

I believe ARM64 based NS2 that uses the same iProc PCIe driver might also need something like this (I'm still waiting for Jon Mason to give me some more information on the NS2 errors that he saw, which is likely related to this). I assume there will be something similar to the ARM specific "hook_fault_code" for ARM64, but then ARM64 does not have any "mach" specific directory. Where can this type of hook be installed for ARM64 based platforms if not in the PCIe driver?

OK, that is a fair point, then maybe have a two stage process, where we make sure that the part that installs the hook is always available and built-into the kernel, but let the iProc PCIe driver remain a module?

Quoted 8 lines

Quoted 7 lines

- code is ways built-in, and hook_fault_code is installed prior to PCIe loading (function is marked with __init)
- platforms which do not need that, just do not install it for that specific code
- it is clear which platforms need it and which do not, yet the driver remains agnostic

Quoted 13 lines

It's actually unclear to me which iProc platforms need this.
- We know NS needs it
- Someone mentioned NSP does not need this? But where does the information come from? Do we have similar PCIe connection configuration on NSP as NS?
- I cannot confirm whether Cygnus needs this or not since we do not have similar board configurations on Cygnus (on Cygnus each host is always connected to a single device). I can check with the ASIC team, but that will be a very lengthy process and I'm not sure when I can get the answer we need (and yes I do work at Broadcom, :))
- Jon Mason reported similar abort issues on NS2 when an externally connected, multi function device is used. I'm still waiting for him to send me the error log and see if it's the same type of data abort.

Is there really no way to flip some chicken bits to prevent the PCIe root complex from forwarding some of these errors?

Also, sorry if this is repeating information, but how can we generate spurious aborts if we are scanning the PCIe bus, are we possibly trying to access the RC's config space before it is set up (this should not fail), or something else?

Thanks!

Quoted 46 lines · Florian Fainelli

Quoted 45 lines · Ray Jui

Quoted 41 lines · Florian Fainelli

Quoted 15 lines · Rafa? Mi?ecki

Some devices (e.g. Northstar ones) may have bridges that forward harmless errors to the ARM core. In such case we need an option to add a handler ignoring them.

Signed-off-by Rafa? Mi?ecki <[email protected]>

---

+- brcm,pcie-hook-abort-handler: During PCI bus probing (device enumeration)
+ there can be errors that are expected and harmless. Unfortunately some bridges
+ can't be configured to ignore them and they forward them to the ARM core

(quoted diff) +2 -0

+  triggering die().
+  This property should be set in such case, it will make driver add

Quoted 2 lines

its own
+ handler ignoring such errors.

The property is named after the function that allows you to catch abort handlers, whereas you should be describing the HW here. Something like brcm,bridge-error-forward or a better name even would be preferable.

drivers/pci/host/pcie-iproc.c static int iproc_pcie_map_ranges(struct iproc_pcie *pcie, · line 443

Quoted 9 lines

(quoted diff) +2 -0

+#ifdef CONFIG_ARM
+static int iproc_pcie_abort_handler(unsigned long addr, unsigned int

fsr,

drivers/pci/host/pcie-iproc.c +6 -0

+                    struct pt_regs *regs)
+{
+    if (fsr == 0x1406)
+        return 0;
+
+    return 1;

As you later noted this prevents this driver from being a module now. Since the expectation is that either a fixed bootloader or a platform should enot produce these data aborts, or allow them to be ignored, why not just put this code back where it belongs in the machine specific file which kills many birds with the same stone:

Quoted 7 lines

Quoted 2 lines

I assume the module compile issue can be simply fixed by exporting symbol of "hook_fault_code"?

I do not think it is desireable for this symbol to be exported in the first place, also last I looked, this was a one time registration thing, you cannot undo the hook you installed, but everything can be fixed.

Okay.

Quoted 9 lines

Quoted 8 lines

I believe ARM64 based NS2 that uses the same iProc PCIe driver might also need something like this (I'm still waiting for Jon Mason to give me some more information on the NS2 errors that he saw, which is likely related to this). I assume there will be something similar to the ARM specific "hook_fault_code" for ARM64, but then ARM64 does not have any "mach" specific directory. Where can this type of hook be installed for ARM64 based platforms if not in the PCIe driver?

Quoted 4 lines

OK, that is a fair point, then maybe have a two stage process, where we make sure that the part that installs the hook is always available and built-into the kernel, but let the iProc PCIe driver remain a module?

I guess we are sort of stuck on this. If "hook_fault_code" is not supposed to be used by any driver compiled as module like you described, then yes, I agree, I don't see how we can leave this in the iProc PCIe driver.

One Comment below

Quoted 74 lines · Ray Jui

Quoted 47 lines · Florian Fainelli

Quoted 46 lines · Ray Jui

Quoted 42 lines · Florian Fainelli

Quoted 16 lines · Rafa? Mi?ecki

Some devices (e.g. Northstar ones) may have bridges that forward harmless errors to the ARM core. In such case we need an option to add a handler ignoring them.

Signed-off-by Rafa? Mi?ecki <[email protected]>

---

+- brcm,pcie-hook-abort-handler: During PCI bus probing (device enumeration)
+ there can be errors that are expected and harmless. Unfortunately some bridges
+ can't be configured to ignore them and they forward them to the ARM
core

(quoted diff) +2 -0

+  triggering die().
+  This property should be set in such case, it will make driver add

Quoted 2 lines

its own
+ handler ignoring such errors.

The property is named after the function that allows you to catch abort handlers, whereas you should be describing the HW here. Something like brcm,bridge-error-forward or a better name even would be preferable.

drivers/pci/host/pcie-iproc.c static int iproc_pcie_map_ranges(struct iproc_pcie *pcie, · line 443

Quoted 9 lines

(quoted diff) +2 -0

+#ifdef CONFIG_ARM
+static int iproc_pcie_abort_handler(unsigned long addr, unsigned int

fsr,

drivers/pci/host/pcie-iproc.c +6 -0

+                    struct pt_regs *regs)
+{
+    if (fsr == 0x1406)
+        return 0;
+
+    return 1;

As you later noted this prevents this driver from being a module now. Since the expectation is that either a fixed bootloader or a platform should enot produce these data aborts, or allow them to be ignored, why not just put this code back where it belongs in the machine specific file which kills many birds with the same stone:

Quoted 7 lines

Quoted 2 lines

I assume the module compile issue can be simply fixed by exporting symbol of "hook_fault_code"?

I do not think it is desireable for this symbol to be exported in the first place, also last I looked, this was a one time registration thing, you cannot undo the hook you installed, but everything can be fixed.

Okay.

Quoted 9 lines

Quoted 8 lines

I believe ARM64 based NS2 that uses the same iProc PCIe driver might also need something like this (I'm still waiting for Jon Mason to give me some more information on the NS2 errors that he saw, which is likely related to this). I assume there will be something similar to the ARM specific "hook_fault_code" for ARM64, but then ARM64 does not have any "mach" specific directory. Where can this type of hook be installed for ARM64 based platforms if not in the PCIe driver?

Quoted 4 lines

OK, that is a fair point, then maybe have a two stage process, where we make sure that the part that installs the hook is always available and built-into the kernel, but let the iProc PCIe driver remain a module?

Quoted 5 lines

I guess we are sort of stuck on this. If "hook_fault_code" is not supposed to be used by any driver compiled as module like you described, then yes, I agree, I don't see how we can leave this in the iProc PCIe driver.

Why does thie PCI driver need to be compiled as module though? Why can't we limit it to being linked in the kernel?

Regards,
Scott

Quoted 86 lines · Scott Branden

One Comment below

Quoted 78 lines · Ray Jui

Quoted 48 lines · Florian Fainelli

Quoted 47 lines · Ray Jui

Quoted 43 lines · Florian Fainelli

Quoted 16 lines · Rafa? Mi?ecki

Some devices (e.g. Northstar ones) may have bridges that forward harmless errors to the ARM core. In such case we need an option to add a handler ignoring them.

Signed-off-by Rafa? Mi?ecki <[email protected]>

---

+- brcm,pcie-hook-abort-handler: During PCI bus probing (device enumeration)
+ there can be errors that are expected and harmless. Unfortunately some bridges
+ can't be configured to ignore them and they forward them to the ARM
core

(quoted diff) +2 -0

+  triggering die().
+  This property should be set in such case, it will make driver add

Quoted 2 lines

its own
+ handler ignoring such errors.

The property is named after the function that allows you to catch abort handlers, whereas you should be describing the HW here. Something like brcm,bridge-error-forward or a better name even would be preferable.

Review

Quoted 10 lines

(quoted diff) +2 -0

+#ifdef CONFIG_ARM
+static int iproc_pcie_abort_handler(unsigned long addr, unsigned

int
fsr,

(quoted diff) +6 -0

+                    struct pt_regs *regs)
+{
+    if (fsr == 0x1406)
+        return 0;
+
+    return 1;

As you later noted this prevents this driver from being a module now. Since the expectation is that either a fixed bootloader or a platform should enot produce these data aborts, or allow them to be ignored, why not just put this code back where it belongs in the machine specific file which kills many birds with the same stone:

Quoted 8 lines

Quoted 2 lines

I assume the module compile issue can be simply fixed by exporting symbol of "hook_fault_code"?

I do not think it is desireable for this symbol to be exported in the first place, also last I looked, this was a one time registration thing,
you cannot undo the hook you installed, but everything can be fixed.

Okay.

Quoted 11 lines

Quoted 10 lines

I believe ARM64 based NS2 that uses the same iProc PCIe driver might also need something like this (I'm still waiting for Jon Mason to give me some more information on the NS2 errors that he saw, which is likely
related to this). I assume there will be something similar to the ARM specific "hook_fault_code" for ARM64, but then ARM64 does not have any "mach" specific directory. Where can this type of hook be installed for
ARM64 based platforms if not in the PCIe driver?

Quoted 4 lines

OK, that is a fair point, then maybe have a two stage process, where we make sure that the part that installs the hook is always available and built-into the kernel, but let the iProc PCIe driver remain a module?

Quoted 5 lines

I guess we are sort of stuck on this. If "hook_fault_code" is not supposed to be used by any driver compiled as module like you described, then yes, I agree, I don't see how we can leave this in the iProc PCIe driver.

Quoted 5 lines

Why does thie PCI driver need to be compiled as module though? Why can't we limit it to being linked in the kernel?

Regards,
Scott

There are minor benefits allowing this driver to be compiled as module, although in our use case (Cygnus and NS2), we always compile this driver as statically linked in the kernel. I'm not sure if NS/BCMA has any use case that requires this driver to be a module.

In fact, being able to compile this driver as a module and loaded after kernel init process is done just helped to confirm this imprecise abort issue to be PCIe specific, :)

Quoted 95 lines · Ray Jui

Quoted 86 lines · Scott Branden

One Comment below

Quoted 78 lines · Ray Jui

Quoted 48 lines · Florian Fainelli

Quoted 47 lines · Ray Jui

Quoted 43 lines · Florian Fainelli

Quoted 16 lines · Rafa? Mi?ecki

Some devices (e.g. Northstar ones) may have bridges that forward harmless errors to the ARM core. In such case we need an option to add a handler ignoring them.

Signed-off-by Rafa? Mi?ecki <[email protected]>

---

+- brcm,pcie-hook-abort-handler: During PCI bus probing (device enumeration)
+ there can be errors that are expected and harmless. Unfortunately some bridges
+ can't be configured to ignore them and they forward them to the ARM
core

(quoted diff) +2 -0

+  triggering die().
+  This property should be set in such case, it will make driver add

Quoted 2 lines

its own
+ handler ignoring such errors.

The property is named after the function that allows you to catch abort handlers, whereas you should be describing the HW here. Something like brcm,bridge-error-forward or a better name even would be preferable.

Review

Quoted 10 lines

(quoted diff) +2 -0

+#ifdef CONFIG_ARM
+static int iproc_pcie_abort_handler(unsigned long addr, unsigned

int
fsr,

(quoted diff) +6 -0

+                    struct pt_regs *regs)
+{
+    if (fsr == 0x1406)
+        return 0;
+
+    return 1;

As you later noted this prevents this driver from being a module now. Since the expectation is that either a fixed bootloader or a platform should enot produce these data aborts, or allow them to be ignored, why not just put this code back where it belongs in the machine specific file which kills many birds with the same stone:

Quoted 8 lines

Quoted 2 lines

I assume the module compile issue can be simply fixed by exporting symbol of "hook_fault_code"?

I do not think it is desireable for this symbol to be exported in the first place, also last I looked, this was a one time registration thing,
you cannot undo the hook you installed, but everything can be fixed.

Okay.

Quoted 11 lines

Quoted 10 lines

I believe ARM64 based NS2 that uses the same iProc PCIe driver might also need something like this (I'm still waiting for Jon Mason to give me some more information on the NS2 errors that he saw, which is likely
related to this). I assume there will be something similar to the ARM specific "hook_fault_code" for ARM64, but then ARM64 does not have any "mach" specific directory. Where can this type of hook be installed for
ARM64 based platforms if not in the PCIe driver?

Quoted 4 lines

OK, that is a fair point, then maybe have a two stage process, where we make sure that the part that installs the hook is always available and built-into the kernel, but let the iProc PCIe driver remain a module?

Quoted 5 lines

I guess we are sort of stuck on this. If "hook_fault_code" is not supposed to be used by any driver compiled as module like you described, then yes, I agree, I don't see how we can leave this in the iProc PCIe driver.

Quoted 5 lines

Why does thie PCI driver need to be compiled as module though? Why can't we limit it to being linked in the kernel?

Regards,
Scott

Quoted 8 lines

There are minor benefits allowing this driver to be compiled as module, although in our use case (Cygnus and NS2), we always compile this driver as statically linked in the kernel. I'm not sure if NS/BCMA has any use case that requires this driver to be a module.

In fact, being able to compile this driver as a module and loaded after kernel init process is done just helped to confirm this imprecise abort issue to be PCIe specific, :)

For the STB platforms for instance, we actually like to have the PCIE RC driver as a module (not iproc-pcie, still downstream for now) because it allows us to put the entire set of EPs and RCs into the lowest power state (L23 + turning off external regulators) without messing with PCI bus scanning. In general, it seems like a good practice to allow this since, that helps with distributions not having to ship gigantic kernels that need this driver built in, and also helps us with development (when faults are handled).

My suggestion about the PCIe-specific hook fault handler is to do something like this: introduce a iproc-pcie-fault.c file which is built-into the kernel if PCIE_IPROC=m || PCIE_IPROC=y and have it contain a function which is hooked at arch_initcall level for instance, so before module_init.

Of course, there could be different ways to solve that problem, it just happens to be one of them.

Quoted 125 lines · Florian Fainelli

Quoted 106 lines · Ray Jui

Quoted 97 lines · Scott Branden

One Comment below

Quoted 88 lines · Ray Jui

Quoted 53 lines · Florian Fainelli

Quoted 52 lines · Ray Jui

Quoted 48 lines · Florian Fainelli

Quoted 17 lines · Rafa? Mi?ecki

Some devices (e.g. Northstar ones) may have bridges that forward harmless errors to the ARM core. In such case we need an option to add a handler ignoring them.

Signed-off-by Rafa? Mi?ecki <[email protected]>

---

+- brcm,pcie-hook-abort-handler: During PCI bus probing (device enumeration)
+ there can be errors that are expected and harmless. Unfortunately
some bridges
+ can't be configured to ignore them and they forward them to the ARM
core

(quoted diff) +2 -0

+  triggering die().
+  This property should be set in such case, it will make

Quoted 3 lines

driver add
its own
+ handler ignoring such errors.

The property is named after the function that allows you to catch abort handlers, whereas you should be describing the HW here. Something like brcm,bridge-error-forward or a better name even would
be preferable.

Review

Quoted 10 lines

(quoted diff) +2 -0

+#ifdef CONFIG_ARM
+static int iproc_pcie_abort_handler(unsigned long addr, unsigned

int
fsr,

(quoted diff) +6 -0

+                    struct pt_regs *regs)
+{
+    if (fsr == 0x1406)
+        return 0;
+
+    return 1;

As you later noted this prevents this driver from being a module now.
Since the expectation is that either a fixed bootloader or a platform
should enot produce these data aborts, or allow them to be ignored, why not just put this code back where it belongs in the machine specific file which kills many birds with the same stone:

Quoted 8 lines

Quoted 2 lines

I assume the module compile issue can be simply fixed by exporting symbol of "hook_fault_code"?

I do not think it is desireable for this symbol to be exported in the first place, also last I looked, this was a one time registration thing,
you cannot undo the hook you installed, but everything can be fixed.

Okay.

Quoted 14 lines

Quoted 13 lines

I believe ARM64 based NS2 that uses the same iProc PCIe driver might also need something like this (I'm still waiting for Jon Mason to give
me some more information on the NS2 errors that he saw, which is likely
related to this). I assume there will be something similar to the ARM
specific "hook_fault_code" for ARM64, but then ARM64 does not have any
"mach" specific directory. Where can this type of hook be installed for
ARM64 based platforms if not in the PCIe driver?

Quoted 6 lines

OK, that is a fair point, then maybe have a two stage process, where we
make sure that the part that installs the hook is always available and
built-into the kernel, but let the iProc PCIe driver remain a module?

Quoted 6 lines

I guess we are sort of stuck on this. If "hook_fault_code" is not supposed to be used by any driver compiled as module like you described,
then yes, I agree, I don't see how we can leave this in the iProc PCIe driver.

Quoted 5 lines

Why does thie PCI driver need to be compiled as module though? Why can't we limit it to being linked in the kernel?

Regards,
Scott

Quoted 8 lines

There are minor benefits allowing this driver to be compiled as module, although in our use case (Cygnus and NS2), we always compile this driver as statically linked in the kernel. I'm not sure if NS/BCMA has any use case that requires this driver to be a module.

In fact, being able to compile this driver as a module and loaded after kernel init process is done just helped to confirm this imprecise abort issue to be PCIe specific, :)

For the STB platforms for instance, we actually like to have the PCIE RC driver as a module (not iproc-pcie, still downstream for now) because it allows us to put the entire set of EPs and RCs into the lowest power state (L23 + turning off external regulators) without messing with PCI bus scanning. In general, it seems like a good practice to allow this since, that helps with distributions not having to ship gigantic kernels that need this driver built in, and also helps us with development (when faults are handled).

Quoted 9 lines

My suggestion about the PCIe-specific hook fault handler is to do something like this: introduce a iproc-pcie-fault.c file which is built-into the kernel if PCIE_IPROC=m || PCIE_IPROC=y and have it contain a function which is hooked at arch_initcall level for instance, so before module_init.

Of course, there could be different ways to solve that problem, it just happens to be one of them.

That sounds good to me.

But how is the hook in iproc-pcie-fault.c activated? Still based on a DT property under the iProc PCIe device node or based on a specific architecture dependent config flag (note we do not have any platform specific config flag under ARM64)?

Quoted 134 lines · Ray Jui

Quoted 123 lines · Florian Fainelli

Quoted 104 lines · Ray Jui

Quoted 97 lines · Scott Branden

One Comment below

Quoted 88 lines · Ray Jui

Quoted 53 lines · Florian Fainelli

Quoted 52 lines · Ray Jui

Quoted 48 lines · Florian Fainelli

Quoted 17 lines · Rafa? Mi?ecki

Some devices (e.g. Northstar ones) may have bridges that forward harmless errors to the ARM core. In such case we need an option to add a handler ignoring them.

Signed-off-by Rafa? Mi?ecki <[email protected]>

---

+- brcm,pcie-hook-abort-handler: During PCI bus probing (device enumeration)
+ there can be errors that are expected and harmless. Unfortunately
some bridges
+ can't be configured to ignore them and they forward them to the ARM
core

(quoted diff) +2 -0

+  triggering die().
+  This property should be set in such case, it will make

Quoted 3 lines

driver add
its own
+ handler ignoring such errors.

The property is named after the function that allows you to catch abort handlers, whereas you should be describing the HW here. Something like brcm,bridge-error-forward or a better name even would
be preferable.

Review

Quoted 10 lines

(quoted diff) +2 -0

+#ifdef CONFIG_ARM
+static int iproc_pcie_abort_handler(unsigned long addr, unsigned

int
fsr,

(quoted diff) +6 -0

+                    struct pt_regs *regs)
+{
+    if (fsr == 0x1406)
+        return 0;
+
+    return 1;

As you later noted this prevents this driver from being a module now.
Since the expectation is that either a fixed bootloader or a platform
should enot produce these data aborts, or allow them to be ignored, why not just put this code back where it belongs in the machine specific file which kills many birds with the same stone:

Quoted 8 lines

Quoted 2 lines

I assume the module compile issue can be simply fixed by exporting symbol of "hook_fault_code"?

I do not think it is desireable for this symbol to be exported in the first place, also last I looked, this was a one time registration thing,
you cannot undo the hook you installed, but everything can be fixed.

Okay.

Quoted 14 lines

Quoted 13 lines

I believe ARM64 based NS2 that uses the same iProc PCIe driver might also need something like this (I'm still waiting for Jon Mason to give
me some more information on the NS2 errors that he saw, which is likely
related to this). I assume there will be something similar to the ARM
specific "hook_fault_code" for ARM64, but then ARM64 does not have any
"mach" specific directory. Where can this type of hook be installed for
ARM64 based platforms if not in the PCIe driver?

Quoted 6 lines

OK, that is a fair point, then maybe have a two stage process, where we
make sure that the part that installs the hook is always available and
built-into the kernel, but let the iProc PCIe driver remain a module?

Quoted 6 lines

I guess we are sort of stuck on this. If "hook_fault_code" is not supposed to be used by any driver compiled as module like you described,
then yes, I agree, I don't see how we can leave this in the iProc PCIe driver.

Quoted 5 lines

Why does thie PCI driver need to be compiled as module though? Why can't we limit it to being linked in the kernel?

Regards,
Scott

Quoted 8 lines

There are minor benefits allowing this driver to be compiled as module, although in our use case (Cygnus and NS2), we always compile this driver as statically linked in the kernel. I'm not sure if NS/BCMA has any use case that requires this driver to be a module.

In fact, being able to compile this driver as a module and loaded after kernel init process is done just helped to confirm this imprecise abort issue to be PCIe specific, :)

For the STB platforms for instance, we actually like to have the PCIE RC driver as a module (not iproc-pcie, still downstream for now) because it allows us to put the entire set of EPs and RCs into the lowest power state (L23 + turning off external regulators) without messing with PCI bus scanning. In general, it seems like a good practice to allow this since, that helps with distributions not having to ship gigantic kernels that need this driver built in, and also helps us with development (when faults are handled).

Quoted 9 lines

My suggestion about the PCIe-specific hook fault handler is to do something like this: introduce a iproc-pcie-fault.c file which is built-into the kernel if PCIE_IPROC=m || PCIE_IPROC=y and have it contain a function which is hooked at arch_initcall level for instance, so before module_init.

Of course, there could be different ways to solve that problem, it just happens to be one of them.

Quoted 6 lines

That sounds good to me.

But how is the hook in iproc-pcie-fault.c activated? Still based on a DT property under the iProc PCIe device node or based on a specific architecture dependent config flag (note we do not have any platform specific config flag under ARM64)?

I would go with something that does:

if (of_machine_is_compatible("brcm,bcm53012") ||

    of_machine_is_compatible("brcm.ns2"))

or something along these lines, as opposed to a boolean flag at the PCIe DT node level, but maybe this is not quite a good understanding of how the HW works.

drivers/pci/host/pcie-iproc.c static int iproc_pcie_map_ranges(struct iproc_pcie *pcie, · line 443

Quoted 9 lines

(quoted diff) +2 -0

+#ifdef CONFIG_ARM
+static int iproc_pcie_abort_handler(unsigned long addr, unsigned int

fsr,

drivers/pci/host/pcie-iproc.c +6 -0

+                                  struct pt_regs *regs)
+{
+      if (fsr == 0x1406)
+              return 0;
+
+      return 1;

As you later noted this prevents this driver from being a module now. Since the expectation is that either a fixed bootloader or a platform should enot produce these data aborts, or allow them to be ignored, why not just put this code back where it belongs in the machine specific file which kills many birds with the same stone:

- code is ways built-in, and hook_fault_code is installed prior to PCIe loading (function is marked with __init)
- platforms which do not need that, just do not install it for that specific code
- it is clear which platforms need it and which do not, yet the driver remains agnostic

NB: there could be other platforms some day needing that which also propagate the error differently, forcing you to add more and more of these codes in the PCIe driver.

Quoted 12 lines · On Sunday 10 April 2016 18:43:52 Florian Fainelli wrote:

drivers/pci/host/pcie-iproc.c static int iproc_pcie_map_ranges(struct iproc_pcie *pcie, · line 443

Quoted 9 lines

(quoted diff) +2 -0

+#ifdef CONFIG_ARM
+static int iproc_pcie_abort_handler(unsigned long addr, unsigned int

fsr,

drivers/pci/host/pcie-iproc.c +6 -0

+                                  struct pt_regs *regs)
+{
+      if (fsr == 0x1406)
+              return 0;
+
+      return 1;

As you later noted this prevents this driver from being a module now. Since the expectation is that either a fixed bootloader or a platform should enot produce these data aborts, or allow them to be ignored, why not just put this code back where it belongs in the machine specific file which kills many birds with the same stone:

Quoted 6 lines

- code is ways built-in, and hook_fault_code is installed prior to PCIe loading (function is marked with __init)
- platforms which do not need that, just do not install it for that specific code
- it is clear which platforms need it and which do not, yet the driver remains agnostic

NB: there could be other platforms some day needing that which also propagate the error differently, forcing you to add more and more of these codes in the PCIe driver.

I think ideally the driver should be able to access some of its internal registers to figure out what really happened, but the handler above doesn't do that, it just silently ignores *any* errors based on the fsr.

Could one of you check the datasheets for the iproc PCI hardware to see if there are any error handling registers we may want to use to further drill down on what went wrong and whether it is safe to ignore the CPU fault?

Arnd

Quoted 34 lines · Arnd Bergmann

Quoted 23 lines · On Sunday 10 April 2016 18:43:52 Florian Fainelli wrote:

drivers/pci/host/pcie-iproc.c static int iproc_pcie_map_ranges(struct iproc_pcie *pcie, · line 443

Quoted 9 lines

(quoted diff) +2 -0

+#ifdef CONFIG_ARM
+static int iproc_pcie_abort_handler(unsigned long addr, unsigned int

fsr,

drivers/pci/host/pcie-iproc.c +6 -0

+                                  struct pt_regs *regs)
+{
+      if (fsr == 0x1406)
+              return 0;
+
+      return 1;

As you later noted this prevents this driver from being a module now. Since the expectation is that either a fixed bootloader or a platform should enot produce these data aborts, or allow them to be ignored, why not just put this code back where it belongs in the machine specific file which kills many birds with the same stone:

- code is ways built-in, and hook_fault_code is installed prior to PCIe loading (function is marked with __init)
- platforms which do not need that, just do not install it for that specific code
- it is clear which platforms need it and which do not, yet the driver remains agnostic

Quoted 4 lines

NB: there could be other platforms some day needing that which also propagate the error differently, forcing you to add more and more of these codes in the PCIe driver.

I think ideally the driver should be able to access some of its internal registers to figure out what really happened, but the handler above doesn't
do that, it just silently ignores *any* errors based on the fsr.

Quoted 4 lines

Could one of you check the datasheets for the iproc PCI hardware to see if there are any error handling registers we may want to use to further drill down on what went wrong and whether it is safe to ignore the CPU fault?

I'm still trying... I've never worked on NorthStar but I believe the PAXB PCIe block in NorthStar is essentially the same (or at least very similar) to NSP and Cygnus. I couldn't find any registers from the Cygnus datasheet that allows us to either disable this abort or provide a mechanism for better error handling (at least there's nothing obvious from the datasheet of Cygnus).

I'm trying to contact the ASIC designer of this block and see if I can get further information from him.

Thanks,

Ray

Quoted 3 lines

Arnd

Hi Rafal/Florian/Arnd,

After a couple days of email exchange with the ASIC team, I think I've figured out the behavior on all of the Broadcom SoCs that use this iProc PCIe controller.

On NSP, Cygnus, and NS2:
- There's an APB error enable register at offset 0xf40 from the iProc PCIe controller's base address. If one clears bit 0 (enabled by default after chip POR) of that register, one can stop this from being forwarded to "iProc host" as an APB error/external imprecise abort
- I will submit a patch to the iProc PCIe driver to disable this error forwarding

On NS:
- Unfortunately, there's no such control register in NS. In other words, we cannot disable this error at the PCIe controller level
- FSR code corresponds to external (bit[12] = '1'), read (bit[11] = '0'), imprecise abort (bits[10][3:0] = '1''0110'), i.e., external imprecise abort triggered by read access. Our ASIC team believes a read access to a non-exist APB register can also trigger an abort with the same FSR code. Note this is the tricky part, by registering an abort hook that skips this particular FSR, one has a chance of skipping other aborts triggered by accessing invalid APB registers. But given that this cannot be disabled for the PCIe controller NS, I'm not sure what approach we should take. Any thoughts?

From Ray's e-mail it seems Northstar is the only platform requiring

Quoted 14 lines · Ray Jui

Hi Rafal/Florian/Arnd,

After a couple days of email exchange with the ASIC team, I think I've figured out the behavior on all of the Broadcom SoCs that use this iProc PCIe controller.

On NSP, Cygnus, and NS2:
- There's an APB error enable register at offset 0xf40 from the iProc PCIe controller's base address. If one clears bit 0 (enabled by default after chip POR) of that register, one can stop this from being forwarded to "iProc host" as an APB error/external imprecise abort
- I will submit a patch to the iProc PCIe driver to disable this error forwarding

Quoted 12 lines

On NS:
- Unfortunately, there's no such control register in NS. In other words, we cannot disable this error at the PCIe controller level
- FSR code corresponds to external (bit[12] = '1'), read (bit[11] = '0'), imprecise abort (bits[10][3:0] = '1''0110'), i.e., external imprecise abort triggered by read access. Our ASIC team believes a read access to a non-exist APB register can also trigger an abort with the same FSR code. Note this is the tricky part, by registering an abort hook that skips this particular FSR, one has a chance of skipping other aborts triggered by accessing invalid APB registers. But given that this cannot be disabled for the PCIe controller NS, I'm not sure what approach we should take. Any thoughts?

It's really late reply but I wanted to finally handle this problem.

From Ray's e-mail it seems Northstar is the only platform requiring this workaround. So we don't have to worry about arm64.

We have two options then:
1) Add workaround in arch/arm/mach-bcm/bcm_5301x.c
2) Add workaround into built-in drivers/pci/host/pcie-iproc-fault.c

Do you have any preference about that?

Hi Rafal,

Quoted 28 lines · Rafa? Mi?ecki

Quoted 14 lines · Ray Jui

Hi Rafal/Florian/Arnd,

After a couple days of email exchange with the ASIC team, I think I've figured out the behavior on all of the Broadcom SoCs that use this iProc PCIe controller.

On NSP, Cygnus, and NS2:
- There's an APB error enable register at offset 0xf40 from the iProc PCIe controller's base address. If one clears bit 0 (enabled by default after chip POR) of that register, one can stop this from being forwarded to "iProc host" as an APB error/external imprecise abort
- I will submit a patch to the iProc PCIe driver to disable this error forwarding

Quoted 12 lines

On NS:
- Unfortunately, there's no such control register in NS. In other words, we cannot disable this error at the PCIe controller level
- FSR code corresponds to external (bit[12] = '1'), read (bit[11] = '0'), imprecise abort (bits[10][3:0] = '1''0110'), i.e., external imprecise abort triggered by read access. Our ASIC team believes a read access to a non-exist APB register can also trigger an abort with the same FSR code. Note this is the tricky part, by registering an abort hook that skips this particular FSR, one has a chance of skipping other aborts triggered by accessing invalid APB registers. But given that this cannot be disabled for the PCIe controller NS, I'm not sure what approach we should take. Any thoughts?

Quoted 4 lines

It's really late reply but I wanted to finally handle this problem.

From Ray's e-mail it seems Northstar is the only platform requiring this workaround. So we don't have to worry about arm64.

Yes, Northstar is the only platform that requires this workaround. Even the arm32 platforms like NSP and Cygnus can disable unsupported request being forwarded as APB error. I've recently sent out a patch series to fix this for all other platforms, and sorry I should have included you in the email but I did not. I'll include you when revision 2 is sent out.

Quoted 1 line

Quoted 3 lines

We have two options then:
1) Add workaround in arch/arm/mach-bcm/bcm_5301x.c
2) Add workaround into built-in drivers/pci/host/pcie-iproc-fault.c

How do you plan to implement pcie-iproc-fault.c? If it's similar to what you have now, then I think it fits more to bcm_5301x.c

Quoted 28 lines · Rafa? Mi?ecki

Quoted 14 lines · Ray Jui

Hi Rafal/Florian/Arnd,

After a couple days of email exchange with the ASIC team, I think I've figured out the behavior on all of the Broadcom SoCs that use this iProc PCIe controller.

On NSP, Cygnus, and NS2:
- There's an APB error enable register at offset 0xf40 from the iProc PCIe controller's base address. If one clears bit 0 (enabled by default after chip POR) of that register, one can stop this from being forwarded to "iProc host" as an APB error/external imprecise abort
- I will submit a patch to the iProc PCIe driver to disable this error forwarding

Quoted 12 lines

On NS:
- Unfortunately, there's no such control register in NS. In other words, we cannot disable this error at the PCIe controller level
- FSR code corresponds to external (bit[12] = '1'), read (bit[11] = '0'), imprecise abort (bits[10][3:0] = '1''0110'), i.e., external imprecise abort triggered by read access. Our ASIC team believes a read access to a non-exist APB register can also trigger an abort with the same FSR code. Note this is the tricky part, by registering an abort hook that skips this particular FSR, one has a chance of skipping other aborts triggered by accessing invalid APB registers. But given that this cannot be disabled for the PCIe controller NS, I'm not sure what approach we should take. Any thoughts?

Quoted 4 lines

It's really late reply but I wanted to finally handle this problem.

From Ray's e-mail it seems Northstar is the only platform requiring this workaround. So we don't have to worry about arm64.

Yes, Northstar is the only platform that requires this workaround. Even the arm32 platforms like NSP and Cygnus can disable unsupported request being forwarded as APB error. I've recently sent out a patch series to fix this for all other platforms, and sorry I should have included you in the email but I did not. I'll include you when revision 2 is sent out.

Quoted 1 line

Quoted 3 lines

We have two options then:
1) Add workaround in arch/arm/mach-bcm/bcm_5301x.c
2) Add workaround into built-in drivers/pci/host/pcie-iproc-fault.c

How do you plan to implement pcie-iproc-fault.c? If it's similar to what you have now, then I think it fits more to bcm_5301x.c

Some devices (e.g. Northstar ones) may have bridges that forward harmless errors to the ARM core. In such case we need an option to add a handler ignoring them.

Signed-off-by Rafa? Mi?ecki <[email protected]>

---
 .../devicetree/bindings/pci/brcm,iproc-pcie.txt         |  6 ++++++
 drivers/pci/host/pcie-iproc-platform.c                  |  2 ++
 drivers/pci/host/pcie-iproc.c                           | 17 +++++++++++++++++
 drivers/pci/host/pcie-iproc.h                           |  1 +
 4 files changed, 26 insertions(+)

Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt +0 -0

index 01b88f4..c91b20a 100644

Please Cc the device tree mailing list (devicetree at vger.kernel.org) when sending device tree patches.

Quoted 18 lines

Some devices (e.g. Northstar ones) may have bridges that forward harmless errors to the ARM core. In such case we need an option to add a handler ignoring them.

Signed-off-by Rafa? Mi?ecki <[email protected]>

---
 .../devicetree/bindings/pci/brcm,iproc-pcie.txt         |  6 ++++++
 drivers/pci/host/pcie-iproc-platform.c                  |  2 ++
 drivers/pci/host/pcie-iproc.c                           | 17 +++++++++++++++++
 drivers/pci/host/pcie-iproc.h                           |  1 +
 4 files changed, 26 insertions(+)

Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt +0 -0

index 01b88f4..c91b20a 100644
@@ -22,6 +22,12 @@ Optional properties:
 

Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt Optional properties: · line 30

Quoted 8 lines

Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt +6 -0

 - brcm,pcie-ob: Some iProc SoCs do not have the outbound address mapping done
 by the ASIC after power on reset. In this case, SW needs to configure it
+- brcm,pcie-hook-abort-handler: During PCI bus probing (device enumeration)
+  there can be errors that are expected and harmless. Unfortunately some bridges
+  can't be configured to ignore them and they forward them to the ARM core
+  triggering die().
+  This property should be set in such case, it will make driver add its own
+  handler ignoring such errors.

Rather than describing what the kernel should do, this should describe the property of the hardware (e.g. this should be named something like brcm,spurious-probing-abort).