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