[PATCH 0/2] Two small INTx fixes for Rockchip's dwc based PCIe controller driver

[PATCH 0/2] Two small INTx fixes for Rockchip's dwc based PCIe controller driver

[Shawn Lin]

This small series fixes two INTx related issues in the Rockchip
DesignWare PCIe controller driver.

Note that patch 2 depends on b376b3ff9cb0 which is not yet in Linus'
tree, so this series is based on linux-next.



Shawn Lin (2):
  PCI: dw-rockchip: Bail out if the INTx irq domain creation fails
  PCI: dw-rockchip: Do not recreate the INTx irq domain on root port
    reset

 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 41 +++++++++++++++++----------
 1 file changed, 26 insertions(+), 15 deletions(-)

-- 
2.7.4


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH 1/2] PCI: dw-rockchip: Bail out if the INTx irq domain creation fails

[Shawn Lin]

rockchip_pcie_init_irq_domain() can fail, e.g. when the DT node is
missing its "legacy-interrupt-controller" child node. In that case,
rockchip_pcie_host_init() only prints an error and continues, arming the
chained INTx handler with a NULL irq_domain. The first INTx interrupt
then dereferences the NULL irq_domain in rockchip_pcie_intx_handler(),
which panics the machine.

Propagate the error to abort probing, keeping the error message for
easier debugging.

Fixes: e8aae154df61 ("PCI: rockchip-dwc: Add legacy interrupt support")
Cc: Peter Geis <pgwipeout@gmail.com>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index af26a07..50824c8 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -432,8 +432,10 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
 	pci->dbi_base2 = pci->dbi_base + PCIE_TYPE0_HDR_DBI2_OFFSET;
 
 	ret = rockchip_pcie_init_irq_domain(rockchip);
-	if (ret < 0)
+	if (ret < 0) {
 		dev_err(dev, "failed to init irq domain\n");
+		return ret;
+	}
 
 	irq_set_chained_handler_and_data(irq, rockchip_pcie_intx_handler,
 					 rockchip);
-- 
2.7.4


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH 2/2] PCI: dw-rockchip: Do not recreate the INTx irq domain on root port reset

[Shawn Lin]

.reset_root_port() re-runs the host ops .init() callback to reprogram
the Root Complex after the controller reset. However, .init() also
creates a new INTx irq domain on every root port reset, so that:

  - the previous irq domain is leaked, as it is never removed, and two
    irq domains end up registered for the same fwnode;

  - the INTx virqs of the downstream PCI devices were allocated in the
    previous irq domain and are never re-mapped, while the chained
    handler now looks up virqs in the new, empty domain. Hence, after a
    link down recovery, INTx interrupts are silently lost.

Split the (re)programming of the Root Complex registers out of .init()
into rockchip_pcie_host_hw_init() and call that from .reset_root_port()
instead. The INTx irq domain and the chained handler are now only set up
once, at probe time, which keeps the already mapped virqs valid across
root port resets.

Fixes: b376b3ff9cb0 ("PCI: dw-rockchip: Implement .reset_root_port() and use for link down")
Cc: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 37 +++++++++++++++++----------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 50824c8..ff92394 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -418,6 +418,27 @@ static void rockchip_pcie_stop_link(struct dw_pcie *pci)
 	rockchip_pcie_ltssm_trace(rockchip, false);
 }
 
+/*
+ * (Re)program the Root Complex registers that are cleared by the controller
+ * reset. Called from .init() at probe time and from .reset_root_port().
+ * The INTx irq domain must not be touched here, as downstream devices hold
+ * virqs mapped in it.
+ */
+static void rockchip_pcie_host_hw_init(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+	pci->dbi_base2 = pci->dbi_base + PCIE_TYPE0_HDR_DBI2_OFFSET;
+
+	rockchip_pcie_configure_l1ss(pci);
+	rockchip_pcie_enable_l0s(pci);
+	pp->bridge->reset_root_port = rockchip_pcie_rc_reset_root_port;
+
+	/* Disable Root Ports BAR0 and BAR1 as they report bogus size */
+	dw_pcie_writel_dbi2(pci, PCI_BASE_ADDRESS_0, 0x0);
+	dw_pcie_writel_dbi2(pci, PCI_BASE_ADDRESS_1, 0x0);
+}
+
 static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
@@ -429,8 +450,6 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
 	if (irq < 0)
 		return irq;
 
-	pci->dbi_base2 = pci->dbi_base + PCIE_TYPE0_HDR_DBI2_OFFSET;
-
 	ret = rockchip_pcie_init_irq_domain(rockchip);
 	if (ret < 0) {
 		dev_err(dev, "failed to init irq domain\n");
@@ -440,13 +459,7 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
 	irq_set_chained_handler_and_data(irq, rockchip_pcie_intx_handler,
 					 rockchip);
 
-	rockchip_pcie_configure_l1ss(pci);
-	rockchip_pcie_enable_l0s(pci);
-	pp->bridge->reset_root_port = rockchip_pcie_rc_reset_root_port;
-
-	/* Disable Root Ports BAR0 and BAR1 as they report bogus size */
-	dw_pcie_writel_dbi2(pci, PCI_BASE_ADDRESS_0, 0x0);
-	dw_pcie_writel_dbi2(pci, PCI_BASE_ADDRESS_1, 0x0);
+	rockchip_pcie_host_hw_init(pp);
 
 	return 0;
 }
@@ -920,11 +933,7 @@ static int rockchip_pcie_rc_reset_root_port(struct pci_host_bridge *bridge,
 	if (ret)
 		goto deinit_phy;
 
-	ret = pp->ops->init(pp);
-	if (ret) {
-		dev_err(dev, "Host init failed: %d\n", ret);
-		goto deinit_clk;
-	}
+	rockchip_pcie_host_hw_init(pp);
 
 	/* LTSSM enable control mode */
 	val = FIELD_PREP_WM16(PCIE_LTSSM_ENABLE_ENHANCE, 1);
-- 
2.7.4


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 0/2] Two small INTx fixes for Rockchip's dwc based PCIe controller driver

[Niklas Cassel]

Hello Shawn,

On Thu, Sep 03, 2026 at 10:44:35AM +0800, Shawn Lin wrote:
> 
> This small series fixes two INTx related issues in the Rockchip
> DesignWare PCIe controller driver.
> 
> Note that patch 2 depends on b376b3ff9cb0 which is not yet in Linus'
> tree, so this series is based on linux-next.

$ git tag --contains b376b3ff9cb0
v7.3-rc1

FWIW, the commit you are referencing is in Linus' tree already.


Kind regards,
Niklas

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 0/2] Two small INTx fixes for Rockchip's dwc based PCIe controller driver

[Shawn Lin]

在 2026/09/03 星期四 16:20, Niklas Cassel 写道:
> Hello Shawn,
> 
> On Thu, Sep 03, 2026 at 10:44:35AM +0800, Shawn Lin wrote:
>>
>> This small series fixes two INTx related issues in the Rockchip
>> DesignWare PCIe controller driver.
>>
>> Note that patch 2 depends on b376b3ff9cb0 which is not yet in Linus'
>> tree, so this series is based on linux-next.
> 
> $ git tag --contains b376b3ff9cb0
> v7.3-rc1
> 
> FWIW, the commit you are referencing is in Linus' tree already.

Ah, a few days ago I didn't send out the patch in time, but now it is 
indeed on Linus' tree.

Thanks.

> 
> 
> Kind regards,
> Niklas
> 


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 2/2] PCI: dw-rockchip: Do not recreate the INTx irq domain on root port reset

[Niklas Cassel]

On Thu, Sep 03, 2026 at 10:44:37AM +0800, Shawn Lin wrote:
> .reset_root_port() re-runs the host ops .init() callback to reprogram
> the Root Complex after the controller reset. However, .init() also
> creates a new INTx irq domain on every root port reset, so that:
> 
>   - the previous irq domain is leaked, as it is never removed, and two
>     irq domains end up registered for the same fwnode;
> 
>   - the INTx virqs of the downstream PCI devices were allocated in the
>     previous irq domain and are never re-mapped, while the chained
>     handler now looks up virqs in the new, empty domain. Hence, after a
>     link down recovery, INTx interrupts are silently lost.
> 
> Split the (re)programming of the Root Complex registers out of .init()
> into rockchip_pcie_host_hw_init() and call that from .reset_root_port()
> instead. The INTx irq domain and the chained handler are now only set up
> once, at probe time, which keeps the already mapped virqs valid across
> root port resets.
> 
> Fixes: b376b3ff9cb0 ("PCI: dw-rockchip: Implement .reset_root_port() and use for link down")
> Cc: Niklas Cassel <cassel@kernel.org>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> 
> ---

If I compare to pcie-qcom.c, the difference is that they do
e.g.:
irq = platform_get_irq_byname_optional(pdev, "global");

after calling dw_pcie_host_init() in qcom_pcie_probe().


I guess pcie-dw-rockchip.c could do the same:
Call of_irq_get_byname() and rockchip_pcie_init_irq_domain() in
rockchip_pcie_configure_rc(), after calling dw_pcie_host_init().

That way, you don't need to introduce another rockchip_pcie_host_hw_init().

pci->pp.ops->init() is called by both dw_pcie_host_init() and
dw_pcie_resume_noirq(). So calling of_irq_get_byname() in .init()
does seem slightly wrong, as we would get the irq on each resume.

Perhaps pcie-dw-rockchip.c does not have support for resume, so it
does not matter right now, but still seems a bit weird to call
of_irq_get_byname() in init().

I did not look if rockchip_pcie_init_irq_domain() should be called
on each resume, but I since we don't tear down the irq_domain in
pci->pp.ops->deinit(), in fact we don't even have a ->deinit(),
so calling rockchip_pcie_init_irq_domain() in ->init() does seem
wrong as well.

So my vote is to move both to rockchip_pcie_configure_rc(), after
calling dw_pcie_host_init().


Kind regards,
Niklas

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 2/2] PCI: dw-rockchip: Do not recreate the INTx irq domain on root port reset

[Shawn Lin]

Hi Niklas

在 2026/09/03 星期四 16:37, Niklas Cassel 写道:
> On Thu, Sep 03, 2026 at 10:44:37AM +0800, Shawn Lin wrote:
>> .reset_root_port() re-runs the host ops .init() callback to reprogram
>> the Root Complex after the controller reset. However, .init() also
>> creates a new INTx irq domain on every root port reset, so that:
>>
>>    - the previous irq domain is leaked, as it is never removed, and two
>>      irq domains end up registered for the same fwnode;
>>
>>    - the INTx virqs of the downstream PCI devices were allocated in the
>>      previous irq domain and are never re-mapped, while the chained
>>      handler now looks up virqs in the new, empty domain. Hence, after a
>>      link down recovery, INTx interrupts are silently lost.
>>
>> Split the (re)programming of the Root Complex registers out of .init()
>> into rockchip_pcie_host_hw_init() and call that from .reset_root_port()
>> instead. The INTx irq domain and the chained handler are now only set up
>> once, at probe time, which keeps the already mapped virqs valid across
>> root port resets.
>>
>> Fixes: b376b3ff9cb0 ("PCI: dw-rockchip: Implement .reset_root_port() and use for link down")
>> Cc: Niklas Cassel <cassel@kernel.org>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>
>> ---
> 
> If I compare to pcie-qcom.c, the difference is that they do
> e.g.:
> irq = platform_get_irq_byname_optional(pdev, "global");
> 
> after calling dw_pcie_host_init() in qcom_pcie_probe().
> 
> 
> I guess pcie-dw-rockchip.c could do the same:
> Call of_irq_get_byname() and rockchip_pcie_init_irq_domain() in
> rockchip_pcie_configure_rc(), after calling dw_pcie_host_init().
> 
> That way, you don't need to introduce another rockchip_pcie_host_hw_init().
> 
> pci->pp.ops->init() is called by both dw_pcie_host_init() and
> dw_pcie_resume_noirq(). So calling of_irq_get_byname() in .init()
> does seem slightly wrong, as we would get the irq on each resume.
> 
> Perhaps pcie-dw-rockchip.c does not have support for resume, so it
> does not matter right now, but still seems a bit weird to call
> of_irq_get_byname() in init().
> 
> I did not look if rockchip_pcie_init_irq_domain() should be called
> on each resume, but I since we don't tear down the irq_domain in
> pci->pp.ops->deinit(), in fact we don't even have a ->deinit(),
> so calling rockchip_pcie_init_irq_domain() in ->init() does seem
> wrong as well.
> 
> So my vote is to move both to rockchip_pcie_configure_rc(), after
> calling dw_pcie_host_init().
> 

Thanks for the review!

I agree with your reasoning. Will moves the of_irq_get_byname() lookup,
the INTx irq domain creation and the chained handler installation into
rockchip_pcie_configure_rc(), right after dw_pcie_host_init(), matching
the qcom pattern you pointed out. The rockchip_pcie_host_hw_init()
helper from will be gone in v2, and the host ops .init() callback is
back to doing only idempotent register programming, which
.reset_root_port() can safely re-run.

Since there is still no ->deinit() to pair with .init(), and the
Sashiko review also flagged the missing cleanup on probe failure, I
made the irq domain devm-managed (devm_irq_domain_instantiate()) and
uninstall the chained handler through a devres action while moving the
code, so everything is released automatically if probe fails.

Will send out v2 for review.

> 
> Kind regards,
> Niklas
> 


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip