[RESEND] Re: [PATCH] PCI: dw-rockchip: Skip waiting for link up

[FUKAUMI Naoki]

On 11/11/25 04:59, Niklas Cassel wrote:
> On Mon, Nov 10, 2025 at 09:23:02PM +0530, Manivannan Sadhasivam wrote:
>>>
>>> Considering what Shawn says, that the switch gets enumerated properly
>>> if we simply add a msleep() in ->start_link(), which will be called
>>> by dw_pcie_host_init() before pci_host_probe() is called...
>>>
>>
>> Yes, that delay probably gives enough time for the link up IRQ to kick in before
>> the initial bus scan happens.
> 
> I think that the problem is that even for platforms with link up IRQ,
> we will always do:
> 1) dw_pcie_start_link() (if (!dw_pcie_link_up()))
> 2) pci_host_probe() from dw_pcie_host_init(), this will enumerate the bus
> 3) pci_rescan_bus() from the Link Up IRQ handler
> 
> Thus, in 2, when enumerating the bus, without performing any of the delays
> mandated by the PCIe spec, it still seems possible to detect a device (it
> must have been really quick to initialize), and to communicate with that
> device, however since we have not performed the delays mandated by the spec,
> it appears that the device might not yet behave properly.
> 
> Hence my suggestion to never call pci_host_probe() in dw_pcie_host_init()
> for platforms with Link Up IRQ.
> 
> At least for pcie-dw-rockchip.c, we only unmask the Link Up IRQ after
> dw_pcie_host_init() has returned, so I think that your theory: that Shawn's
> suggested delay causes the Link Up IRQ to kick in before the initial bus
> scan, is incorrect. (Since the IRQ should not be able to trigger until
> dw_pcie_host_init() has returned.)
> 
> 
>>
>>> ...we already have a delay in the link up IRQ handler, before calling
>>> pci_rescan_bus().
>>>
>>
>> That delay won't help in this case.
> 
> Sure, I was just saying that even though Shawn's patch made things work,
> it seems incorrect, as we do not want to add "the same delay" that we
> already have in the Link Up IRQ. (The delay in the Link Up IRQ should be
> the only one.)
> 
> 
>>> I think a better solution would be something like:
> 
> (snip)
> 
>> This solution will work as long as the PCIe device is powered ON before
>> start_link(). For CEM and M.2 Key M connectors, the host controller can power
>> manage the components. But for other specifications/keys requiring custom power
>> management, a separate driver would be needed.
>>
>> That's why I suggested using pwrctrl framework as it can satisfy both usecases.
>> However, as I said, it needs a bit of rework and I'm close to submitting it.
>>
>> But until that gets merged, either we need to revert your link up IRQ change or
>> have the below patch. IMO, the revert seems simple.
> 
> Using pwrctrl framework once it can handle this use case sounds good to me.
> 
> FUKAUMI, could you please send a revert of the two patches?

Leaving the commit message aside, I'm currently testing with a revert of 
the two patches.

Vanilla v6.18-rc5, CONFIG_PCI_DYNAMIC_OF_NODES=y, revert ec9fd499b9c6, 
revert 0e0b45ab5d77.

It works stably on the ROCK 5A. The link speed is 2Gb/s.

The ROCK 5C is unstable. It initially worked with a link speed of 4Gb/s, 
but eventually started showing kernel oops. The dts files for the 5A and 
5C are compatible and interchangeable, but even using the 5A's dts on 
the 5C, the operation remains unstable.

I plan to thoroughly investigate the ROCK 5C's behavior on v6.13, but 
for now, I believe reverting the two patches is the correct action.

Best regards,

--
FUKAUMI Naoki
Radxa Computer (Shenzhen) Co., Ltd.

> Kind regards,
> Niklas
>