Re: [PATCH] PCI: rcar-gen4: Fix inverted break condition in PHY initialization

[Marek Vasut <marek.vasut@mailbox.org>]

On 9/16/25 7:13 PM, Bjorn Helgaas wrote:
> On Tue, Sep 16, 2025 at 06:31:31PM +0200, Marek Vasut wrote:
>> On 9/16/25 3:57 PM, Geert Uytterhoeven wrote:
>>> On Tue, 16 Sept 2025 at 15:39, Marek Vasut <marek.vasut@mailbox.org> wrote:
>>>> On 9/16/25 11:59 AM, Geert Uytterhoeven wrote:
> 
>>>>> This change looks correct, and fixes the timeout seen on White Hawk
>>>>> with CONFIG_DEBUG_LOCK_ALLOC=y.
>>>>> However, it causes a crash when CONFIG_DEBUG_LOCK_ALLOC=n:
>>>>>
>>>>>        SError Interrupt on CPU0, code 0x00000000be000011 -- SError
>>>>
>>>> ...
>>>>
>>>>>         el1h_64_error_handler+0x2c/0x40
>>>>>         el1h_64_error+0x70/0x74
>>>>>         dw_pcie_read+0x20/0x74 (P)
>>>>>         rcar_gen4_pcie_additional_common_init+0x1c/0x6c
>>>>
>>>> SError in rcar_gen4_pcie_additional_common_init , this is unrelated to
>>>> this fix.
>>>>
>>>> Does the following patch make this SError go away ?
>>>
>>>> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
>>>> @@ -204,6 +204,8 @@ static int rcar_gen4_pcie_common_init(struct
>>>> rcar_gen4_pcie *rcar)
>>>>            if (ret)
>>>>                    goto err_unprepare;
>>>>
>>>> +mdelay(1);
>>>> +
>>>>            if (rcar->drvdata->additional_common_init)
>>>>                    rcar->drvdata->additional_common_init(rcar);
>>>>
>>>
>>> Yes it does, thanks!
>>
>> So with this one extra mdelay(1), the PCIe is fully good on your side, or is
>> there still anything weird/flaky/malfunctioning ?
> 
> Do we have a theory about why this delay is needed?  I assume it's
> something specific to the rcar hardware (not something required by the
> PCIe base spec)?
> 
> I'm seeing SError interrupts and external aborts on several arm64
> systems and I'd like to understand better why they happen and when/if
> we can recover from them.
The SError here happens when the PWR RST is released and DBI is accessed 
rapidly right after that. The current hypothesis is, that the controller 
core needs a bit of time to initialize itself after the reset is 
released, before DBI access can be performed ; if the DBI access happens 
too soon after the reset was released, the core reject the access and 
CPU interprets that as SError.

The reference manual however does not list any such delay, which makes 
me concerned. I can send such a patch which adds the mdelay(1) as a 
temporary stopgap fix, until some better explanation maybe sometimes 
gets uncovered, if that would be OK ?