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

Shawn Lin
2025-11-11T01:11:56Z

Hi Mani

在 2025/11/10 星期一 23:53, Manivannan Sadhasivam 写道:
> On Mon, Nov 10, 2025 at 01:34:41PM +0100, Niklas Cassel wrote:
>> On Mon, Nov 10, 2025 at 06:15:33PM +0800, Shawn Lin wrote:
>>>>
>>>> Could you try PCIe 2.0 slot on your board?
>>>
>>> I did, it doesn't work on PCIe 2.0 slot. From the PA, I could see
>>> the link is still in training during pci_host_probe() is called.
>>> Add some delay before pci_rescan_bus() in pcie-dw-rockchip doesn't
>>> help. But the below change should work as we delayed pci_host_probe().
>>>
>>> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>> @@ -236,6 +236,8 @@ static int rockchip_pcie_start_link(struct dw_pcie *pci)
>>> msleep(PCIE_T_PVPERL_MS);
>>> gpiod_set_value_cansleep(rockchip->rst_gpio, 1);
>>>
>>> + msleep(50);
>>> +
>>> return 0;
>>>
>>> Otherwise we got:
>>>
>>> [ 0.841518] pci_bus 0003:33: busn_res: can not insert [bus 33-31] under
>>> [bus 32-31] (conflicts with (null) [bus 32-31])
>>> [ 0.842596] pci_bus 0003:33: busn_res: [bus 33-31] end is updated to 33
>>> [ 0.843184] pci_bus 0003:33: busn_res: can not insert [bus 33] under [bus
>>> 32-31] (conflicts with (null) [bus 32-31])
>>> [ 0.844120] pci 0003:32:00.0: devices behind bridge are unusable because
>>> [bus 33] cannot be assigned for them
>>> [ 0.845229] pci_bus 0003:34: busn_res: can not insert [bus 34-31] under
>>> [bus 32-31] (conflicts with (null) [bus 32-31])
>>> [ 0.846309] pci_bus 0003:34: busn_res: [bus 34-31] end is updated to 34
>>> [ 0.846898] pci_bus 0003:34: busn_res: can not insert [bus 34] under [bus
>>> 32-31] (conflicts with (null) [bus 32-31])
>>> [ 0.847833] pci 0003:32:06.0: devices behind bridge are unusable because
>>> [bus 34] cannot be assigned for them
>>> [ 0.848923] pci_bus 0003:35: busn_res: can not insert [bus 35-31] under
>>> [bus 32-31] (conflicts with (null) [bus 32-31])
>>> [ 0.850014] pci_bus 0003:35: busn_res: [bus 35-31] end is updated to 35
>>> [ 0.850605] pci_bus 0003:35: busn_res: can not insert [bus 35] under [bus
>>> 32-31] (conflicts with (null) [bus 32-31])
>>> [ 0.851540] pci 0003:32:0e.0: devices behind bridge are unusable because
>>> [bus 35] cannot be assigned for them
>>> [ 0.852424] pci_bus 0003:32: busn_res: [bus 32-31] end is updated to 35
>>> [ 0.853028] pci_bus 0003:32: busn_res: can not insert [bus 32-35] under
>>> [bus 31] (conflicts with (null) [bus 31])
>>> [ 0.853184] hub 3-0:1.0: USB hub found
>>> [ 0.853931] pci 0003:31:00.0: devices behind bridge are unusable because
>>> [bus 32-35] cannot be assigned for them
>>> [ 0.854262] hub 3-0:1.0: 1 port detected
>>> [ 0.855144] pcieport 0003:30:00.0: bridge has subordinate 31 but max busn
>>> 35
>>> [ 0.855722] hub 4-0:1.0: USB hub found
>>> [ 0.856109] pci 0003:32:00.0: PCI bridge to [bus 33]
>>> [ 0.856939] pci 0003:32:06.0: PCI bridge to [bus 34]
>>> [ 0.857133] hub 4-0:1.0: 1 port detected
>>> [ 0.857430] pci 0003:32:0e.0: PCI bridge to [bus 35]
>>> [ 0.858236] pci 0003:31:00.0: PCI bridge to [bus 32-35]
>>
>> Mani,
>>
>> while I see the idea behind your suggested hack:
>>
>> + if (pdev->vendor == 0x1d87 && pdev->device == 0x3588) {
>> + pdev->is_hotplug_bridge = pdev->is_pciehp = 1;
>> + return;
>> + }
>>
>>
>> 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.
>
>> ...we already have a delay in the link up IRQ handler, before calling
>> pci_rescan_bus().
>>
>
> That delay won't help in this case.
>
>> So, I think that the problem is that we are unconditionally calling
>> pci_host_probe() in dw_pcie_host_init(), even for platforms that have
>> a link-up IRQ.
>>
>>
>> I think a better solution would be something like:
>>
>
> 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.
>

My test environment is a PCIe2.0 x1 CEM slot and a PCIe3.0 x4 CEM slot.
The power supply for slots are controlled by pcie-dw-rockchip, far
earlier than staring to link. From this point, I would expect the result
is consistent, both worked or both failed. But, the result for these two
slots is different, PCIe2.0 slot didn't work but PCIe3.0 slot worked.

After checking with PA, with gpio-input-tigger's help, I could see
PCIe2.0 is still in tranning with thet switch, during pci_host_probe()
is called. This is the only difference I could see then, so as a debug I
add a delay to postpone the pci_host_probe() to make sure the link will
be finished on background.

So this's true if I added the delay, the pci_host_probe（） will see the
switch and the following devices. So your analysis of extented bus makes
sense. But pwrctrl seems helpless here, as we sure power-on the slot
before star_link().

> 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.
>
> - Mani
>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>> index e92513c5bda5..42d987ddab7d 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>> @@ -565,6 +565,39 @@ static int dw_pcie_host_get_resources(struct dw_pcie_rp *pp)
>> return 0;
>> }
>>
>> +static int dw_pcie_host_initial_scan(struct dw_pcie_rp *pp)
>> +{
>> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> + struct pci_host_bridge *bridge = pp->bridge;
>> + int ret;
>> +
>> + ret = pci_host_probe(bridge);
>> + if (ret)
>> + return ret;
>> +
>> + if (pp->ops->post_init)
>> + pp->ops->post_init(pp);
>> +
>> + dwc_pcie_debugfs_init(pci, DW_PCIE_RC_TYPE);
>> +
>> + return 0;
>> +}
>> +
>> +void dw_pcie_handle_link_up_irq(struct dw_pcie_rp *pp)
>> +{
>> + if (!pp->initial_linkup_irq_done) {
>> + if (dw_pcie_host_initial_scan(pp)) {
>> + //TODO: cleanup
>> + }
>> + pp->initial_linkup_irq_done = true;
>> + } else {
>> + /* Rescan the bus to enumerate endpoint devices */
>> + pci_lock_rescan_remove();
>> + pci_rescan_bus(pp->bridge->bus);
>> + pci_unlock_rescan_remove();
>> + }
>> +}
>> +
>> int dw_pcie_host_init(struct dw_pcie_rp *pp)
>> {
>> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> @@ -672,15 +705,13 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>> if (!pp->use_linkup_irq)
>> /* Ignore errors, the link may come up later */
>> dw_pcie_wait_for_link(pci);
>> -
>> - ret = pci_host_probe(bridge);
>> - if (ret)
>> - goto err_stop_link;
>> -
>> - if (pp->ops->post_init)
>> - pp->ops->post_init(pp);
>> -
>> - dwc_pcie_debugfs_init(pci, DW_PCIE_RC_TYPE);
>> + else
>> + /*
>> + * For platforms with Link Up IRQ, initial scan will be done
>> + * on first Link Up IRQ.
>> + */
>> + if (dw_pcie_host_initial_scan(pp))
>> + goto err_stop_link;
>>
>> return 0;
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>> index e995f692a1ec..a31bd93490dc 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -427,6 +427,7 @@ struct dw_pcie_rp {
>> int msg_atu_index;
>> struct resource *msg_res;
>> bool use_linkup_irq;
>> + bool initial_linkup_irq_done;
>> struct pci_eq_presets presets;
>> struct pci_config_window *cfg;
>> bool ecam_enabled;
>> @@ -807,6 +808,7 @@ void dw_pcie_msi_init(struct dw_pcie_rp *pp);
>> int dw_pcie_msi_host_init(struct dw_pcie_rp *pp);
>> void dw_pcie_free_msi(struct dw_pcie_rp *pp);
>> int dw_pcie_setup_rc(struct dw_pcie_rp *pp);
>> +void dw_pcie_handle_link_up_irq(struct dw_pcie_rp *pp);
>> int dw_pcie_host_init(struct dw_pcie_rp *pp);
>> void dw_pcie_host_deinit(struct dw_pcie_rp *pp);
>> int dw_pcie_allocate_domains(struct dw_pcie_rp *pp);
>> @@ -844,6 +846,9 @@ static inline int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
>> return 0;
>> }
>>
>> +static inline void dw_pcie_handle_link_up_irq(struct dw_pcie_rp *pp)
>> +{ }
>> +
>> static inline int dw_pcie_host_init(struct dw_pcie_rp *pp)
>> {
>> return 0;
>> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>> index 8a882dcd1e4e..042e5845bdd6 100644
>> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>> @@ -468,10 +468,7 @@ static irqreturn_t rockchip_pcie_rc_sys_irq_thread(int irq, void *arg)
>> if (rockchip_pcie_link_up(pci)) {
>> msleep(PCIE_RESET_CONFIG_WAIT_MS);
>> dev_dbg(dev, "Received Link up event. Starting enumeration!\n");
>> - /* Rescan the bus to enumerate endpoint devices */
>> - pci_lock_rescan_remove();
>> - pci_rescan_bus(pp->bridge->bus);
>> - pci_unlock_rescan_remove();
>> + dw_pcie_handle_link_up_irq(pp);
>> }
>> }
>>
>>
>>
>>
>>
>> What do you think?
>>
>>
>>
>> Kind regards,
>> Niklas
>