[PATCH v2 1/4] PCI: dw-rockchip: Do not enumerate bus before endpoint devices are ready

[Wilfred Mallawa]

On Tue, 2025-05-06 at 09:39 +0200, Niklas Cassel wrote:
> Commit ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we
> can
> detect Link Up") changed so that we no longer call
> dw_pcie_wait_for_link(),
> and instead enumerate the bus when receiving a Link Up IRQ.
> 
> Laszlo Fiat reported (off-list) that his PLEXTOR PX-256M8PeGN NVMe
> SSD is
> no longer functional, and simply reverting commit ec9fd499b9c6 ("PCI:
> dw-rockchip: Don't wait for link since we can detect Link Up") makes
> his
> SSD functional again.
> 
> It seems that we are enumerating the bus before the endpoint is
> ready.
> Adding a msleep(PCIE_T_RRS_READY_MS) before enumerating the bus in
> the
> threaded IRQ handler makes the SSD functional once again.
> 
> What appears to happen is that before ec9fd499b9c6, we called
> dw_pcie_wait_for_link(), and in the first iteration of the loop, the
> link
> will never be up (because the link was just started),
> dw_pcie_wait_for_link() will then sleep for LINK_WAIT_SLEEP_MS (90
> ms),
> before trying again.
> 
> This means that even if a driver was missing a
> msleep(PCIE_T_RRS_READY_MS)
> (100 ms), because of the call to dw_pcie_wait_for_link(), enumerating
> the
> bus would essentially be delayed by that time anyway (because of the
> sleep
> LINK_WAIT_SLEEP_MS (90 ms)).
> 
> While we could add the msleep(PCIE_T_RRS_READY_MS) after deasserting
> PERST,
> that would essentially bring back an unconditional delay during
> synchronous
> probe (the whole reason to use a Link Up IRQ was to avoid an
> unconditional
> delay during probe).
> 
> Thus, add the msleep(PCIE_T_RRS_READY_MS) before enumerating the bus
> in the
> IRQ handler. This way, we will not have an unconditional delay during
> boot
> for unpopulated PCIe slots.
> 
> Cc: Laszlo Fiat <laszlo.fiat at proton.me>
> Fixes: ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we
> can detect Link Up")
> Signed-off-by: Niklas Cassel <cassel at kernel.org>
> ---
>  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 3c6ab71c996e..6a7ec3545a7f 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -23,6 +23,7 @@
>  #include <linux/reset.h>
>  
>  #include "pcie-designware.h"
> +#include "../../pci.h"
>  
>  /*
>   * The upper 16 bits of PCIE_CLIENT_CONFIG are a write
> @@ -458,6 +459,7 @@ static irqreturn_t
> rockchip_pcie_rc_sys_irq_thread(int irq, void *arg)
>  	if (reg & PCIE_RDLH_LINK_UP_CHGED) {
>  		if (rockchip_pcie_link_up(pci)) {
>  			dev_dbg(dev, "Received Link up event.
> Starting enumeration!\n");
> +			msleep(PCIE_T_RRS_READY_MS);
>  			/* Rescan the bus to enumerate endpoint
> devices */
>  			pci_lock_rescan_remove();
>  			pci_rescan_bus(pp->bridge->bus);
Reviewed-by: Wilfred Mallawa <wilfred.mallawa at wdc.com>