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

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

On 9/16/25 3:57 PM, Geert Uytterhoeven wrote:

Hello Geert,

> 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:
>>> On Tue, 16 Sept 2025 at 01:59, Marek Vasut
>>> <marek.vasut+renesas@mailbox.org> wrote:
>>>> R-Car V4H Reference Manual R19UH0186EJ0130 Rev.1.30 Apr. 21, 2025 page 4581
>>>> Figure 104.3b Initial Setting of PCIEC(example), third quarter of the figure
>>>> indicates that register 0xf8 should be polled until bit 18 becomes set to 1.
>>>>
>>>> Register 0xf8 bit 18 is 0 immediately after write to PCIERSTCTRL1 and is set
>>>> to 1 in less than 1 ms afterward. The current readl_poll_timeout() break
>>>> condition is inverted and returns when register 0xf8 bit 18 is set to 0,
>>>> which in most cases means immediately. In case CONFIG_DEBUG_LOCK_ALLOC=y ,
>>>> the timing changes just enough for the first readl_poll_timeout() poll to
>>>> already read register 0xf8 bit 18 as 1 and afterward never read register
>>>> 0xf8 bit 18 as 0, which leads to timeout and failure to start the PCIe
>>>> controller.
>>>>
>>>> Fix this by inverting the poll condition to match the reference manual
>>>> initialization sequence.
>>>>
>>>> Fixes: faf5a975ee3b ("PCI: rcar-gen4: Add support for R-Car V4H")
>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
>>>
>>> Thanks for your patch!
>>>
>>>> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
>>>> @@ -711,7 +711,7 @@ static int rcar_gen4_pcie_ltssm_control(struct rcar_gen4_pcie *rcar, bool enable
>>>>           val &= ~APP_HOLD_PHY_RST;
>>>>           writel(val, rcar->base + PCIERSTCTRL1);
>>>>
>>>> -       ret = readl_poll_timeout(rcar->phy_base + 0x0f8, val, !(val & BIT(18)), 100, 10000);
>>>> +       ret = readl_poll_timeout(rcar->phy_base + 0x0f8, val, val & BIT(18), 100, 10000);
>>>>           if (ret < 0)
>>>>                   return ret;
>>>>
>>>
>>> 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 ?

If you could give me a RB/TB on this fix, it would be nice.