[PATCH 1/2] PCI: iproc: Support DT property for ignoring aborts when probing

[PATCH 1/2] PCI: iproc: Support DT property for ignoring aborts when probing

[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 <zajec5@gmail.com>
---
 .../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(+)

diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
index 01b88f4..c91b20a 100644
--- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
@@ -22,6 +22,12 @@ Optional properties:
 
 - 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.
 
 If the brcm,pcie-ob property is present, the following properties become
 effective:
diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
index 1738c52..7a6eb09 100644
--- a/drivers/pci/host/pcie-iproc-platform.c
+++ b/drivers/pci/host/pcie-iproc-platform.c
@@ -100,6 +100,8 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
 		pcie->need_ob_cfg = true;
 	}
 
+	pcie->hook_abort_handler = of_property_read_bool(np, "brcm,pcie-hook-abort-handler");
+
 	/* PHY use is optional */
 	pcie->phy = devm_phy_get(&pdev->dev, "pcie-phy");
 	if (IS_ERR(pcie->phy)) {
diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index a576aee..a5b3816 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -433,6 +433,17 @@ static int iproc_pcie_map_ranges(struct iproc_pcie *pcie,
 	return 0;
 }
 
+#ifdef CONFIG_ARM
+static int iproc_pcie_abort_handler(unsigned long addr, unsigned int fsr,
+				    struct pt_regs *regs)
+{
+	if (fsr == 0x1406)
+		return 0;
+
+	return 1;
+}
+#endif
+
 static int iproc_pcie_msi_enable(struct iproc_pcie *pcie)
 {
 	struct device_node *msi_node;
@@ -498,6 +509,12 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
 	}
 
 #ifdef CONFIG_ARM
+	if (pcie->hook_abort_handler)
+		hook_fault_code(16 + 6, iproc_pcie_abort_handler, SIGBUS,
+				BUS_OBJERR, "imprecise external abort");
+#endif
+
+#ifdef CONFIG_ARM
 	pcie->sysdata.private_data = pcie;
 	sysdata = &pcie->sysdata;
 #else
diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
index e84d93c..9a0b58d 100644
--- a/drivers/pci/host/pcie-iproc.h
+++ b/drivers/pci/host/pcie-iproc.h
@@ -72,6 +72,7 @@ struct iproc_pcie {
 	struct phy *phy;
 	int (*map_irq)(const struct pci_dev *, u8, u8);
 	bool need_ob_cfg;
+	bool hook_abort_handler;
 	struct iproc_pcie_ob ob;
 	struct iproc_msi *msi;
 };
-- 
1.8.4.5

[PATCH 2/2] PCI: iproc: Enable hooking abort handler on devices with bcma

[Rafał Miłecki]

This seems to be mostly needed by devices with more than 2 devices, e.g.
Netgear R8000 which is known to suffer from this issue.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 drivers/pci/host/pcie-iproc-bcma.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/host/pcie-iproc-bcma.c b/drivers/pci/host/pcie-iproc-bcma.c
index 0d7bee4..f9ee077 100644
--- a/drivers/pci/host/pcie-iproc-bcma.c
+++ b/drivers/pci/host/pcie-iproc-bcma.c
@@ -56,6 +56,7 @@ static int iproc_pcie_bcma_probe(struct bcma_device *bdev)
 
 	pcie->base = bdev->io_addr;
 	pcie->base_addr = bdev->addr;
+	pcie->hook_abort_handler = true;
 
 	res_mem.start = bdev->addr_s[0];
 	res_mem.end = bdev->addr_s[0] + SZ_128M - 1;
-- 
1.8.4.5

Re: [PATCH 1/2] PCI: iproc: Support DT property for ignoring aborts when probing

[kbuild test robot]

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

Hi Rafał,

[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 >>):

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

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 56979 bytes --]

Re: [PATCH 1/2] PCI: iproc: Support DT property for ignoring aborts when probing

[Rafał Miłecki]

On 10 April 2016 at 04:59, kbuild test robot <lkp@intel.com> wrote:
> [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 >>):
>
>>> ERROR: "hook_fault_code" [drivers/pci/host/pcie-iproc.ko] undefined!

Oh, it seems  hook_fault_code is not an exported SYMBOL and can't be
used when building iproc as module.

Any idea how to resolve this problem?

-- 
Rafał

Re: [PATCH 1/2] PCI: iproc: Support DT property for ignoring aborts when probing

[Florian Fainelli]

On April 9, 2016 2:50:23 PM PDT, "Rafał Miłecki" <zajec5@gmail.com> wrote:
>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 <zajec5@gmail.com>
>---

>+- 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.

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.

>+#ifdef CONFIG_ARM
>+static int iproc_pcie_abort_handler(unsigned long addr, unsigned int
>fsr,
>+				    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.

-- 
Florian

Re: [PATCH 1/2] PCI: iproc: Support DT property for ignoring aborts when probing

[Mark Rutland]

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

On Sat, Apr 09, 2016 at 11:50:23PM +0200, Rafał Miłecki wrote:
> 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 <zajec5@gmail.com>
> ---
>  .../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(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> index 01b88f4..c91b20a 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> @@ -22,6 +22,12 @@ Optional properties:
>  
>  - 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).

Is there absolutely no mechanism to disable this, even if
board-specific?

Are the aborts synchronous or asynchronous?

When specifically do they actually occur?

Thanks,
Mark.

Re: [PATCH 1/2] PCI: iproc: Support DT property for ignoring aborts when probing

[Florian Fainelli]

On 11/04/16 13:06, Ray Jui wrote:
> 
> 
> On 4/10/2016 6:43 PM, Florian Fainelli wrote:
>> On April 9, 2016 2:50:23 PM PDT, "Rafał Miłecki" <zajec5@gmail.com>
>> wrote:
>>> 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 <zajec5@gmail.com>
>>> ---
>>
>>> +- 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.
>>
>> 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.
>>
>>> +#ifdef CONFIG_ARM
>>> +static int iproc_pcie_abort_handler(unsigned long addr, unsigned int
>>> fsr,
>>> +                    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 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.

> 
> 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?

> 
>> - 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.

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!

> 
>>
>> 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


-- 
Florian

Re: [PATCH 1/2] PCI: iproc: Support DT property for ignoring aborts when probing

[Florian Fainelli]

On 11/04/16 15:34, Ray Jui wrote:
> 
> 
> On 4/11/2016 3:26 PM, Scott Branden wrote:
>> One Comment below
>>
>> On 16-04-11 03:24 PM, Ray Jui wrote:
>>>
>>>
>>> On 4/11/2016 2:55 PM, Florian Fainelli wrote:
>>>> On 11/04/16 13:06, Ray Jui wrote:
>>>>>
>>>>>
>>>>> On 4/10/2016 6:43 PM, Florian Fainelli wrote:
>>>>>> On April 9, 2016 2:50:23 PM PDT, "Rafał Miłecki" <zajec5@gmail.com>
>>>>>> wrote:
>>>>>>> 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 <zajec5@gmail.com>
>>>>>>> ---
>>>>>>
>>>>>>> +- 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.
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>>> +#ifdef CONFIG_ARM
>>>>>>> +static int iproc_pcie_abort_handler(unsigned long addr, unsigned
>>>>>>> int
>>>>>>> fsr,
>>>>>>> +                    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 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.
>>>
>>>>>
>>>>> 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?
>>>>
>>>
>>> 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
> 
> 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.
-- 
Florian

Re: [PATCH 1/2] PCI: iproc: Support DT property for ignoring aborts when probing

[Florian Fainelli]

On 11/04/16 15:51, Ray Jui wrote:
> 
> 
> On 4/11/2016 3:41 PM, Florian Fainelli wrote:
>> On 11/04/16 15:34, Ray Jui wrote:
>>>
>>>
>>> On 4/11/2016 3:26 PM, Scott Branden wrote:
>>>> One Comment below
>>>>
>>>> On 16-04-11 03:24 PM, Ray Jui wrote:
>>>>>
>>>>>
>>>>> On 4/11/2016 2:55 PM, Florian Fainelli wrote:
>>>>>> On 11/04/16 13:06, Ray Jui wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 4/10/2016 6:43 PM, Florian Fainelli wrote:
>>>>>>>> On April 9, 2016 2:50:23 PM PDT, "Rafał Miłecki" <zajec5@gmail.com>
>>>>>>>> wrote:
>>>>>>>>> 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 <zajec5@gmail.com>
>>>>>>>>> ---
>>>>>>>>
>>>>>>>>> +- 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.
>>>>>>>>
>>>>>>>> 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.
>>>>>>>>
>>>>>>>>> +#ifdef CONFIG_ARM
>>>>>>>>> +static int iproc_pcie_abort_handler(unsigned long addr, unsigned
>>>>>>>>> int
>>>>>>>>> fsr,
>>>>>>>>> +                    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 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.
>>>>>
>>>>>>>
>>>>>>> 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?
>>>>>>
>>>>>
>>>>> 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
>>>
>>> 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.
>>
> 
> 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.
-- 
Florian

Re: [PATCH 1/2] PCI: iproc: Support DT property for ignoring aborts when probing

[Arnd Bergmann]

On Sunday 10 April 2016 18:43:52 Florian Fainelli wrote:
> >+#ifdef CONFIG_ARM
> >+static int iproc_pcie_abort_handler(unsigned long addr, unsigned int
> >fsr,
> >+                                  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.
> 

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

Re: [PATCH 1/2] PCI: iproc: Support DT property for ignoring aborts when probing

[Rafał Miłecki]

On 11 April 2016 at 10:57, Mark Rutland <mark.rutland@arm.com> wrote:
> Please Cc the device tree mailing list (devicetree@vger.kernel.org) when
> sending device tree patches.

Sorry, I'll remember to do that in future.


> On Sat, Apr 09, 2016 at 11:50:23PM +0200, Rafał Miłecki wrote:
>> 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 <zajec5@gmail.com>
>> ---
>>  .../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(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>> index 01b88f4..c91b20a 100644
>> --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>> @@ -22,6 +22,12 @@ Optional properties:
>>
>>  - 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).

Florian pointed it to me too, I'll use a better property name if we
decide to go this way. Thanks for your comment.

-- 
Rafał

Re: [PATCH 1/2] PCI: iproc: Support DT property for ignoring aborts when probing

[Rafał Miłecki]

On 12 April 2016 at 00:51, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 11/04/16 15:51, Ray Jui wrote:
>> On 4/11/2016 3:41 PM, Florian Fainelli wrote:
>>> On 11/04/16 15:34, Ray Jui wrote:
>>>> On 4/11/2016 3:26 PM, Scott Branden wrote:
>>>>> One Comment below
>>>>>
>>>>> On 16-04-11 03:24 PM, Ray Jui wrote:
>>>>>>
>>>>>>
>>>>>> On 4/11/2016 2:55 PM, Florian Fainelli wrote:
>>>>>>> On 11/04/16 13:06, Ray Jui wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 4/10/2016 6:43 PM, Florian Fainelli wrote:
>>>>>>>>> On April 9, 2016 2:50:23 PM PDT, "Rafał Miłecki" <zajec5@gmail.com>
>>>>>>>>> wrote:
>>>>>>>>>> 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 <zajec5@gmail.com>
>>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>>> +- 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.
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>>> +#ifdef CONFIG_ARM
>>>>>>>>>> +static int iproc_pcie_abort_handler(unsigned long addr, unsigned
>>>>>>>>>> int
>>>>>>>>>> fsr,
>>>>>>>>>> +                    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 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.
>>>>>>
>>>>>>>>
>>>>>>>> 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?
>>>>>>>
>>>>>>
>>>>>> 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
>>>>
>>>> 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.
>>>
>>
>> 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.

Is this design acceptable from DT point of view? I was thinking that
DT is supposed to describe hardware details, it shouldn't be code
storing list of hardware requiring some workarounds.

I guess our iproc-pcie-fault.c could export some symbol that would be
called if a related DT property is set. But after that it would be
simply a workaround of non-exported symbol, I guess it's not something
we want to do.

I also started thinking, what if there will be another driver with
similar hardware issue and it will need to call hook_fault_code as
well? There can be only a one handler registered at a time.

-- 
Rafał

Re: [PATCH 1/2] PCI: iproc: Support DT property for ignoring aborts when probing

[Rafał Miłecki]

On 20 April 2016 at 20:18, Ray Jui <ray.jui@broadcom.com> wrote:
> 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?

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?

Re: [PATCH 1/2] PCI: iproc: Support DT property for ignoring aborts when probing

[Florian Fainelli]

On 10/28/2016 09:58 AM, Ray Jui wrote:
> Hi Rafal,
> 
> On 10/28/2016 8:31 AM, Rafał Miłecki wrote:
>> On 20 April 2016 at 20:18, Ray Jui <ray.jui@broadcom.com> wrote:
>>> 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?
>>
>> 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.
> 
>>
>> 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

I was going to suggest adding it to the PCIe driver so as to make it
localized there, but that seems like a better idea, in case the PCIe
driver is not built into the kernel, or as a module, it seems like a
nice thing to be able to clear the abort.
-- 
Florian

Re: [PATCH 1/2] PCI: iproc: Support DT property for ignoring aborts when probing

[Rafał Miłecki]

On 28 October 2016 at 18:58, Ray Jui <ray.jui@broadcom.com> wrote:
> On 10/28/2016 8:31 AM, Rafa=C5=82 Mi=C5=82ecki wrote:
>> On 20 April 2016 at 20:18, Ray Jui <ray.jui@broadcom.com> wrote:
>>> 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 iPro=
c
>>> PCIe controller.
>>>
>>> On NSP, Cygnus, and NS2:
>>> - There's an APB error enable register at offset 0xf40 from the iProc P=
CIe
>>> controller's base address. If one clears bit 0 (enabled by default afte=
r
>>> 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] =3D '1'), read (bit[11] =3D=
 '0'),
>>> imprecise abort (bits[10][3:0] =3D '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 t=
his
>>> 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.
>
> 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.
>
>>
>> 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

Yes, I just wanted to have a simple file with 2 functions there: one
adding a hook and second being a callback.

--=20
Rafa=C5=82