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

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

On 9/17/25 9:23 AM, Geert Uytterhoeven wrote:

Hello Geert,

> On Tue, 16 Sept 2025 at 18:31, Marek Vasut <marek.vasut@mailbox.org> 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:
>>>>> 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.
> 
> You can have my
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> but only for the combination of both (A) "[PATCH] PCI: rcar-gen4: Fix
> inverted break condition in PHY initialization" and (B) the addition
> of the mdelay.
> 
>    - (A) without (B) causes an SError if CONFIG_DEBUG_LOCK_ALLOC=n,
> 
>    - (B) without (A) causes a timeout if CONFIG_DEBUG_LOCK_ALLOC=n
>      (i.e. same behavior as with CONFIG_DEBUG_LOCK_ALLOC=y).
I have instead posted what I think are proper fixes for that SError:

PCI: rcar-gen4: Add missing 1ms delay after PWR reset assertion
https://patchwork.kernel.org/project/linux-pci/patch/20250918030058.330960-1-marek.vasut+renesas@mailbox.org/

clk: renesas: cpg-mssr: Add missing 1ms delay into reset toggle callback
https://patchwork.kernel.org/project/linux-clk/patch/20250918030552.331389-1-marek.vasut+renesas@mailbox.org/

clk: renesas: cpg-mssr: Read back reset registers to assure values latched
https://patchwork.kernel.org/project/linux-clk/patch/20250918030723.331634-1-marek.vasut+renesas@mailbox.org/

I hope those help. Can you please let me know if they do help ?