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

[Niklas Cassel <cassel@kernel.org>]

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...

...we already have a delay in the link up IRQ handler, before calling
pci_rescan_bus().

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:

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