[PATCH v7 00/14] PCI: rzg3s-host: Add PCIe hotplug support

[PATCH v7 00/14] PCI: rzg3s-host: Add PCIe hotplug support

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Hi,

Series adds hotplug support for the rzg3s-host driver. This is a
continuation of the series John posted initially at [1]. Along with it,
prerequisites fixes and cleanup patches were added.

Series was tested on RZ/G3S and RZ/G3E boards by generating consecutive
link down/up events. The RZ/G3S has an I2C device which, depending on
the way it is programmed, it controls the state of the PCIe link. The
board has 3 PCIe interface: PCIE_SLOT, M.2 Key E, M.2 Key B. The
following script was executed for ~9 hours:

#!/bin/sh

echo performance > /sys/module/pcie_aspm/parameters/policy

while :; do
        i2cset -y 1 0x18 0xf4 0x8 # Link down
        i2cset -y 1 0x18 0xf4 0xc # Link up: PCIE_SLOT (NVMe Micron)
        i2cset -y 1 0x18 0xf4 0x8 # Link down
        i2cset -y 1 0x18 0xf4 0xd # Link up: M.2 Key B (NVMe Transcend)
        i2cset -y 1 0x18 0xf4 0x8 # Link down
        i2cset -y 1 0x18 0xf4 0xb # Link up: M.2 Key E (WiFi AX200)
done

Thank you,
Claudiu

Changes in v7:
- added the following fixes patches:
-- PCI: rzg3s-host: Don't mix goto-based unwinding and cleanup helpers
-- PCI: rzg3s-host: Create the INTx IRQ domain before setting the chained
   handlers
-- PCI: rzg3s-host: Initialize the IRQ domains after the IRQs are masked
-- PCI: rzg3s-host: Drop read-modify-write approach for the R/W1C
   registers
-- PCI: rzg3s-host: Use cached MSI address in .irq_compose_msi_msg()

- addressed sashiko comments:
-- do not mix goto and __free() cleanup constructs
-- addressed concurrency: b/w different paths calling .reset_root_port()
   and b/w IRQ subsystems and .reset_root_port()

Changes in v6:
- addressed review comments as follows:
- added patch 7/9 ("PCI: rzg3s-host: Add helper to wait for link up")
- adopted the approach from pciehp and saved DSN of the connected
  device and, based on it, take decision if the bus devices need to be
  removed + rescanned
- in case of link up and !list_empty(&bus->devices) remove first the
  devices from the bus and re-scan (adopted the approach from pciehp)
- with the new approach cleared the link IRQ (before enabling it) in
  probe, and resume paths

Changes in v5:
- collected tags
- added patch 3/9 ("PCI: rzg3s-host: Select PCI_HOST_COMMON")
- addressed sashiko review comments (detailed in individual patches)

Changes in v4:
- simplified comments in patch 2/8
- kept only DL_UpDown changes in patch 7/8
- added patch 8/8 for the .reset_root_port() support

Changes in v3:
- added patches 1-6
- re-worked patch 7 to use struct pci_host_bridge::reset_root_port() API

[1] https://lore.kernel.org/all/20260630141720.3938514-1-john.madieu.xa@bp.renesas.com/


Claudiu Beznea (13):
  PCI: rzg3s-host: Follow hardware manual clock/reset initialization
    order
  PCI: rzg3s-host: Fix runtime PM handling in the NOIRQ suspend/resume
    phase
  PCI: rzg3s-host: Don't mix goto-based unwinding and cleanup helpers
  PCI: rzg3s-host: Create the INTx IRQ domain before setting the chained
    handlers
  PCI: rzg3s-host: Initialize the IRQ domains after the IRQs are masked
  PCI: rzg3s-host: Drop read-modify-write approach for the R/W1C
    registers
  PCI: rzg3s-host: Use cached MSI address in .irq_compose_msi_msg()
  PCI: rzg3s-host: Drop nop instructions
  PCI: rzg3s-host: Move host configuration code together
  PCI: rzg3s-host: Move suspend/resume code into dedicated functions
  PCI: rzg3s-host: Move IRQ domain setup code
  PCI: rzg3s-host: Add helper to wait for link up
  PCI: rzg3s-host: Add bridge::reset_root_port()

John Madieu (1):
  PCI: rzg3s-host: Re-enumerate the bus on PCIe link-state changes

 drivers/pci/controller/Kconfig           |    1 +
 drivers/pci/controller/pcie-rzg3s-host.c | 1260 +++++++++++++++-------
 2 files changed, 851 insertions(+), 410 deletions(-)

-- 
2.43.0

[PATCH v7 01/14] PCI: rzg3s-host: Follow hardware manual clock/reset initialization order

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The RZ/G3S PCIe hardware manual specifies that the clocks must be enabled
before the reset signals are deasserted during initialization.

Follow this sequence in the probe(), suspend(), and resume() paths to
match the hardware requirements and avoid potential issues.

Fixes: 7ef502fb35b2 ("PCI: Add Renesas RZ/G3S host controller driver")
Cc: stable@vger.kernel.org
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v7:
- none

Changes in v6:
- none

Changes in v5:
- collected tags

Changes in v4:
- none

Changes in v3:
- none, this patch is new

 drivers/pci/controller/pcie-rzg3s-host.c | 40 +++++++++++++-----------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index 077cfb0834b3..e1105b1f2652 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -1888,10 +1888,6 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		goto sysc_signal_restore;
 
-	ret = rzg3s_pcie_power_resets_deassert(host);
-	if (ret)
-		goto sysc_signal_restore;
-
 	pm_runtime_enable(dev);
 
 	/*
@@ -1902,12 +1898,16 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		goto rpm_disable;
 
+	ret = rzg3s_pcie_power_resets_deassert(host);
+	if (ret)
+		goto rpm_put;
+
 	raw_spin_lock_init(&host->hw_lock);
 
 	ret = rzg3s_pcie_host_setup(host, rzg3s_pcie_init_irqdomain,
 				    rzg3s_pcie_teardown_irqdomain);
 	if (ret)
-		goto rpm_put;
+		goto power_resets_assert;
 
 	bridge->sysdata = host;
 	bridge->ops = &rzg3s_pcie_root_ops;
@@ -1922,12 +1922,13 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
 	clk_disable_unprepare(host->port.refclk);
 	rzg3s_pcie_teardown_irqdomain(host);
 	host->data->config_deinit(host);
+power_resets_assert:
+	reset_control_bulk_assert(host->data->num_power_resets,
+				  host->power_resets);
 rpm_put:
 	pm_runtime_put_sync(dev);
 rpm_disable:
 	pm_runtime_disable(dev);
-	reset_control_bulk_assert(host->data->num_power_resets,
-				  host->power_resets);
 sysc_signal_restore:
 	/*
 	 * SYSC RST_RSM_B signal need to be asserted before turning off the
@@ -1948,10 +1949,6 @@ static int rzg3s_pcie_suspend_noirq(struct device *dev)
 	struct rzg3s_sysc *sysc = host->sysc;
 	int ret;
 
-	ret = pm_runtime_put_sync(dev);
-	if (ret)
-		return ret;
-
 	clk_disable_unprepare(port->refclk);
 
 	/* SoC-specific de-initialization */
@@ -1964,13 +1961,19 @@ static int rzg3s_pcie_suspend_noirq(struct device *dev)
 	if (ret)
 		goto config_reinit;
 
-	ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 0);
+	ret = pm_runtime_put_sync(dev);
 	if (ret)
 		goto power_resets_restore;
 
+	ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 0);
+	if (ret)
+		goto rpm_resume;
+
 	return 0;
 
 	/* Restore the previous state if any error happens */
+rpm_resume:
+	pm_runtime_resume_and_get(dev);
 power_resets_restore:
 	reset_control_bulk_deassert(data->num_power_resets,
 				    host->power_resets);
@@ -1980,7 +1983,6 @@ static int rzg3s_pcie_suspend_noirq(struct device *dev)
 	data->config_post_init(host);
 refclk_restore:
 	clk_prepare_enable(port->refclk);
-	pm_runtime_resume_and_get(dev);
 	return ret;
 }
 
@@ -2009,18 +2011,18 @@ static int rzg3s_pcie_resume_noirq(struct device *dev)
 			goto assert_rst_rsm_b;
 	}
 
-	ret = rzg3s_pcie_power_resets_deassert(host);
+	ret = pm_runtime_resume_and_get(dev);
 	if (ret)
 		goto assert_rst_rsm_b;
 
-	ret = pm_runtime_resume_and_get(dev);
+	ret = rzg3s_pcie_power_resets_deassert(host);
 	if (ret)
-		goto assert_power_resets;
+		goto rpm_put;
 
 	ret = rzg3s_pcie_host_setup(host, rzg3s_pcie_msi_hw_setup,
 				    rzg3s_pcie_msi_hw_teardown);
 	if (ret)
-		goto rpm_put;
+		goto assert_power_resets;
 
 	return 0;
 
@@ -2028,11 +2030,11 @@ static int rzg3s_pcie_resume_noirq(struct device *dev)
 	 * If any error happens there is no way to recover the IP. Put it in the
 	 * lowest possible power state.
 	 */
-rpm_put:
-	pm_runtime_put_sync(dev);
 assert_power_resets:
 	reset_control_bulk_assert(data->num_power_resets,
 				  host->power_resets);
+rpm_put:
+	pm_runtime_put_sync(dev);
 assert_rst_rsm_b:
 	rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 0);
 	return ret;
-- 
2.43.0

[PATCH v7 02/14] PCI: rzg3s-host: Fix runtime PM handling in the NOIRQ suspend/resume phase

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The runtime PM documentation states the following:
- During system suspend, pm_runtime_get_noresume() is called for every
  device right before executing the subsystem-level .prepare() callback
  (in device_prepare()). In addition, the PM core disables runtime PM for
  every device right before executing the subsystem-level .suspend_late()
  callback (in device_suspend_late()).
- During system resume, pm_runtime_enable() is called for every device
  right after executing the subsystem-level .resume_early() callback (in
  device_resume_early()), and pm_runtime_put() is called right after
  executing the subsystem-level .complete() callback (in
  device_complete()).

The driver's .suspend_noirq() callback is invoked after .suspend_late(),
while .resume_noirq() is invoked before .resume_early().

If:
- the device is not part of the wake-up path, and
- its runtime PM status is not RPM_SUSPENDED,
the generic power domain .suspend_noirq()/.resume_noirq() callbacks
(genpd_suspend_noirq()/genpd_resume_noirq()) invoke the driver's
.suspend_noirq()/.resume_noirq() callbacks and call
genpd_stop_dev()/genpd_start_dev() before and after them, respectively.

Calling genpd_stop_dev()/genpd_start_dev() allows devices whose power is
controlled by generic power domains to be powered off and on during
system suspend and resume, even though their runtime PM usage count does
not reach zero.

Since the runtime PM usage count is incremented in device_prepare() and
decremented in device_complete(), runtime PM operations performed from
the driver's .suspend_noirq()/.resume_noirq() callbacks are no-ops. The
actual power transitions are handled by the generic power domain
.suspend_noirq()/.resume_noirq() callbacks.

Moreover, attempting to runtime resume a device while runtime PM is
disabled may return -EACCES. This may cause system resume to fail when
resuming after a failed Root Port reset, as described in a subsequent
patch adding hot-plug support.

Remove the runtime PM calls from the driver's
.suspend_noirq()/.resume_noirq() callbacks and rely on the generic power
domain callbacks to power the device off and on.

Fixes: 7ef502fb35b2 ("PCI: Add Renesas RZ/G3S host controller driver")
Cc: stable@vger.kernel.org
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v7:
- none

Changes in v6:
- none

Changes in v5:
- collected tags

Changes in v4:
- simplified the comments in rzg3s_pcie_suspend_noirq()/rzg3s_pcie_resume_noirq()

Changes in v3:
- none, this patch is new

 drivers/pci/controller/pcie-rzg3s-host.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index e1105b1f2652..0ef49bb5ab1a 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -1961,19 +1961,18 @@ static int rzg3s_pcie_suspend_noirq(struct device *dev)
 	if (ret)
 		goto config_reinit;
 
-	ret = pm_runtime_put_sync(dev);
-	if (ret)
-		goto power_resets_restore;
+	/*
+	 * Since the power domain's genpd_suspend_noirq() will disable clocks,
+	 * there is no need to manually invoke runtime PM API here.
+	 */
 
 	ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 0);
 	if (ret)
-		goto rpm_resume;
+		goto power_resets_restore;
 
 	return 0;
 
 	/* Restore the previous state if any error happens */
-rpm_resume:
-	pm_runtime_resume_and_get(dev);
 power_resets_restore:
 	reset_control_bulk_deassert(data->num_power_resets,
 				    host->power_resets);
@@ -2011,13 +2010,14 @@ static int rzg3s_pcie_resume_noirq(struct device *dev)
 			goto assert_rst_rsm_b;
 	}
 
-	ret = pm_runtime_resume_and_get(dev);
-	if (ret)
-		goto assert_rst_rsm_b;
+	/*
+	 * Since the power domain's genpd_resume_noirq() will enable clocks,
+	 * there is no need to manually invoke runtime PM API here.
+	 */
 
 	ret = rzg3s_pcie_power_resets_deassert(host);
 	if (ret)
-		goto rpm_put;
+		goto assert_rst_rsm_b;
 
 	ret = rzg3s_pcie_host_setup(host, rzg3s_pcie_msi_hw_setup,
 				    rzg3s_pcie_msi_hw_teardown);
@@ -2033,8 +2033,6 @@ static int rzg3s_pcie_resume_noirq(struct device *dev)
 assert_power_resets:
 	reset_control_bulk_assert(data->num_power_resets,
 				  host->power_resets);
-rpm_put:
-	pm_runtime_put_sync(dev);
 assert_rst_rsm_b:
 	rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 0);
 	return ret;
-- 
2.43.0

[PATCH v7 03/14] PCI: rzg3s-host: Don't mix goto-based unwinding and cleanup helpers

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

rzg3s_pcie_probe() releases the sysc_np OF node through the
__free(device_node) annotation, while the rest of the function uses
goto-based unwinding. According to cleanup.h, the two styles should
not be mixed within a function, as this makes the ownership semantics
hard to follow. Drop the annotation and call of_node_put() explicitly
on the exit paths instead.

Fixes: 7ef502fb35b2 ("PCI: Add Renesas RZ/G3S host controller driver")
Cc: stable@vger.kernel.org
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v7:
- none, this patch is new

 drivers/pci/controller/pcie-rzg3s-host.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index 0ef49bb5ab1a..bee3e46d025f 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -1822,9 +1822,8 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
 	struct pci_host_bridge *bridge;
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
-	struct device_node *sysc_np __free(device_node) =
-		of_parse_phandle(np, "renesas,sysc", 0);
 	struct rzg3s_pcie_host *host;
+	struct device_node *sysc_np;
 	struct rzg3s_sysc *sysc;
 	int ret;
 
@@ -1859,7 +1858,14 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	sysc_np = of_parse_phandle(np, "renesas,sysc", 0);
+	if (!sysc_np) {
+		ret = -ENODEV;
+		goto port_refclk_put;
+	}
+
 	sysc->regmap = syscon_node_to_regmap(sysc_np);
+	of_node_put(sysc_np);
 	if (IS_ERR(sysc->regmap)) {
 		ret = PTR_ERR(sysc->regmap);
 		goto port_refclk_put;
-- 
2.43.0

[PATCH v7 04/14] PCI: rzg3s-host: Create the INTx IRQ domain before setting the chained handlers

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

rzg3s_pcie_init_irqdomain() installs the chained INTx handlers before
creating the INTx IRQ domain. irq_set_chained_handler_and_data() also
enables the interrupt, so an INTx interrupt arriving in between is
handled by rzg3s_pcie_intx_irq_handler() while host->intx_domain is
still NULL, leading to a NULL pointer dereference.

Move the IRQ domain creation before the loop that installs the chained
handlers.

Fixes: 7ef502fb35b2 ("PCI: Add Renesas RZ/G3S host controller driver")
Cc: stable@vger.kernel.org
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v7:
- none, this patch is new

 drivers/pci/controller/pcie-rzg3s-host.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index bee3e46d025f..cc985020d962 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -1024,6 +1024,16 @@ static int rzg3s_pcie_init_irqdomain(struct rzg3s_pcie_host *host)
 	struct platform_device *pdev = to_platform_device(dev);
 	int i, ret;
 
+	host->intx_domain = irq_domain_create_linear(dev_fwnode(dev),
+						     PCI_NUM_INTX,
+						     &rzg3s_pcie_intx_domain_ops,
+						     host);
+	if (!host->intx_domain) {
+		return dev_err_probe(dev, -EINVAL,
+				     "Failed to add irq domain for INTx IRQs\n");
+	}
+	irq_domain_update_bus_token(host->intx_domain, DOMAIN_BUS_WIRED);
+
 	for (i = 0; i < PCI_NUM_INTX; i++) {
 		char irq_name[5] = {0};
 		int irq;
@@ -1045,18 +1055,6 @@ static int rzg3s_pcie_init_irqdomain(struct rzg3s_pcie_host *host)
 						 host);
 	}
 
-	host->intx_domain = irq_domain_create_linear(dev_fwnode(dev),
-						     PCI_NUM_INTX,
-						     &rzg3s_pcie_intx_domain_ops,
-						     host);
-	if (!host->intx_domain) {
-		ret = -EINVAL;
-		dev_err_probe(dev, ret,
-			      "Failed to add irq domain for INTx IRQs\n");
-		goto teardown_intx;
-	}
-	irq_domain_update_bus_token(host->intx_domain, DOMAIN_BUS_WIRED);
-
 	if (IS_ENABLED(CONFIG_PCI_MSI)) {
 		ret = rzg3s_pcie_init_msi(host);
 
-- 
2.43.0

[PATCH v7 05/14] PCI: rzg3s-host: Initialize the IRQ domains after the IRQs are masked

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The IRQ domain initialization requests the interrupts and installs the
chained handlers before rzg3s_pcie_irq_init() masks the controller
interrupt sources. An interrupt left enabled (e.g. by the bootloader,
or latched from a previous controller state) could therefore fire
before the controller initialization is complete and be handled on
half-initialized state.

Move the IRQ domain initialization after rzg3s_pcie_irq_init(), so that
all the interrupt sources are masked by the time the handlers are
installed.

Fixes: 7ef502fb35b2 ("PCI: Add Renesas RZ/G3S host controller driver")
Cc: stable@vger.kernel.org
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v7:
- none, this patch is new

 drivers/pci/controller/pcie-rzg3s-host.c | 31 ++++++++++++------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index cc985020d962..94345d6f0bee 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -1418,7 +1418,10 @@ static int rzg3s_pcie_host_init_port(struct rzg3s_pcie_host *host)
 	return ret;
 }
 
-static int rzg3s_pcie_host_init(struct rzg3s_pcie_host *host)
+static int
+rzg3s_pcie_host_init(struct rzg3s_pcie_host *host,
+		     int (*init_irqdomain)(struct rzg3s_pcie_host *host),
+		     void (*teardown_irqdomain)(struct rzg3s_pcie_host *host))
 {
 	u32 val;
 	int ret;
@@ -1445,10 +1448,14 @@ static int rzg3s_pcie_host_init(struct rzg3s_pcie_host *host)
 	/* Initialize the interrupts */
 	rzg3s_pcie_irq_init(host);
 
+	ret = init_irqdomain(host);
+	if (ret)
+		goto config_deinit_and_refclk;
+
 	/* SoC-specific post-configuration */
 	ret = host->data->config_post_init(host);
 	if (ret)
-		goto config_deinit_and_refclk;
+		goto teardown_irqdomain;
 
 	/* Wait for link up */
 	ret = readl_poll_timeout(host->axi + RZG3S_PCI_PCSTAT1, val,
@@ -1466,6 +1473,8 @@ static int rzg3s_pcie_host_init(struct rzg3s_pcie_host *host)
 
 config_deinit_post:
 	host->data->config_deinit(host);
+teardown_irqdomain:
+	teardown_irqdomain(host);
 config_deinit_and_refclk:
 	clk_disable_unprepare(host->port.refclk);
 config_deinit:
@@ -1721,15 +1730,10 @@ rzg3s_pcie_host_setup(struct rzg3s_pcie_host *host,
 		return dev_err_probe(dev, ret,
 				     "Failed to set outbound windows!\n");
 
-	ret = init_irqdomain(host);
+	ret = rzg3s_pcie_host_init(host, init_irqdomain, teardown_irqdomain);
 	if (ret)
-		return dev_err_probe(dev, ret, "Failed to init IRQ domain\n");
-
-	ret = rzg3s_pcie_host_init(host);
-	if (ret) {
-		dev_err_probe(dev, ret, "Failed to initialize the HW!\n");
-		goto teardown_irqdomain;
-	}
+		return dev_err_probe(dev, ret,
+				     "Failed to initialize the HW!\n");
 
 	ret = rzg3s_pcie_set_max_link_speed(host);
 	if (ret)
@@ -1738,11 +1742,6 @@ rzg3s_pcie_host_setup(struct rzg3s_pcie_host *host,
 	pci_host_common_link_train_delay(host->max_link_speed);
 
 	return 0;
-
-teardown_irqdomain:
-	teardown_irqdomain(host);
-
-	return ret;
 }
 
 static int rzg3s_pcie_get_controller_id(struct rzg3s_pcie_host *host)
@@ -1923,8 +1922,8 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
 	return 0;
 
 host_probe_teardown:
-	clk_disable_unprepare(host->port.refclk);
 	rzg3s_pcie_teardown_irqdomain(host);
+	clk_disable_unprepare(host->port.refclk);
 	host->data->config_deinit(host);
 power_resets_assert:
 	reset_control_bulk_assert(host->data->num_power_resets,
-- 
2.43.0

[PATCH v7 06/14] PCI: rzg3s-host: Drop read-modify-write approach for the R/W1C registers

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The AXI registers PCI_PINTRCVIS and PCI_PINTRCVIS_MSI are R/W1C. According
to the RZ/G3S HW manual, Rev. 1.30, R/W1C means "it can be cleared to 0b by
writing 1b. Writing 0b does not change anything."

Drop the read-modify-write approach and make use of this hardware
capability. This avoids clearing unwanted interrupts.

Fixes: 7ef502fb35b2 ("PCI: Add Renesas RZ/G3S host controller driver")
Cc: stable@vger.kernel.org
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v7:
- none, this patch is new

 drivers/pci/controller/pcie-rzg3s-host.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index 94345d6f0bee..b0a150262959 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -82,6 +82,7 @@
 #define RZG3S_PCI_MSGRCVIE			0x120
 #define RZG3S_PCI_MSGRCVIE_MSG_RCV		BIT(24)
 
+/* Register is R/W1C, it doesn't require locking. */
 #define RZG3S_PCI_MSGRCVIS			0x124
 #define RZG3S_PCI_MSGRCVIS_MRI			BIT(24)
 
@@ -591,11 +592,10 @@ static irqreturn_t rzg3s_pcie_msi_irq(int irq, void *data)
 		return IRQ_NONE;
 
 	/* Clear the MSI */
-	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_PINTRCVIS,
-			       RZG3S_PCI_PINTRCVIS_MSI,
-			       RZG3S_PCI_PINTRCVIS_MSI);
-	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_MSGRCVIS,
-			       RZG3S_PCI_MSGRCVIS_MRI, RZG3S_PCI_MSGRCVIS_MRI);
+	writel_relaxed(RZG3S_PCI_PINTRCVIS_MSI,
+		       host->axi + RZG3S_PCI_PINTRCVIS);
+	writel_relaxed(RZG3S_PCI_MSGRCVIS_MRI,
+		       host->axi + RZG3S_PCI_MSGRCVIS);
 
 	for (u8 reg_id = 0; reg_id < regs; reg_id++) {
 		status = readl_relaxed(host->axi + RZG3S_PCI_MSIRS(reg_id));
@@ -958,9 +958,8 @@ static void rzg3s_pcie_intx_irq_ack(struct irq_data *d)
 {
 	struct rzg3s_pcie_host *host = irq_data_get_irq_chip_data(d);
 
-	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_PINTRCVIS,
-			       RZG3S_PCI_PINTRCVIS_INTX(d->hwirq),
-			       RZG3S_PCI_PINTRCVIS_INTX(d->hwirq));
+	writel_relaxed(RZG3S_PCI_PINTRCVIS_INTX(d->hwirq),
+		       host->axi + RZG3S_PCI_PINTRCVIS);
 }
 
 static void rzg3s_pcie_intx_irq_mask(struct irq_data *d)
-- 
2.43.0

[PATCH v7 07/14] PCI: rzg3s-host: Use cached MSI address in .irq_compose_msi_msg()

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The MSI window address is already cached in msi->window_base when the
MSI window is configured. Use it to fill msg->address_lo and
msg->address_hi in .irq_compose_msi_msg() instead of reading it back
from the controller registers.

This avoids unnecessary register accesses.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v7:
- none, this patch is new

 drivers/pci/controller/pcie-rzg3s-host.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index b0a150262959..6acb0231a067 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -660,19 +660,9 @@ static void rzg3s_pcie_irq_compose_msi_msg(struct irq_data *data,
 					   struct msi_msg *msg)
 {
 	struct rzg3s_pcie_msi *msi = irq_data_get_irq_chip_data(data);
-	struct rzg3s_pcie_host *host = rzg3s_msi_to_host(msi);
-	u32 lo, hi;
-
-	/*
-	 * Enable and msg data enable bits are part of the address lo. Drop
-	 * them along with the unused bit.
-	 */
-	lo = readl_relaxed(host->axi + RZG3S_PCI_MSIRCVWADRL) &
-	     RZG3S_PCI_MSIRCVWADRL_MASK;
-	hi = readl_relaxed(host->axi + RZG3S_PCI_MSIRCVWADRU);
 
-	msg->address_lo = lo;
-	msg->address_hi = hi;
+	msg->address_lo = lower_32_bits(msi->window_base);
+	msg->address_hi = upper_32_bits(msi->window_base);
 	msg->data = data->hwirq;
 }
 
-- 
2.43.0

[PATCH v7 08/14] PCI: rzg3s-host: Drop nop instructions

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

On the RZ/G3S PCIe IP variant (which is similar to those used on the
RZ/G3E, RZ/V2H, and RZ/V2N SoCs), access to the PCIe Type 1 registers
requires setting the PCI_PERM.CFG_HWINIT_EN bit. Since this bit is not
set on the code paths where rzg3s_pcie_set_max_link_speed() is called,
the writes to PCI_EXP_LNKCTL2.TLS are nops. Drop them.

Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v7:
- none

Changes in v6:
- none

Changes in v5:
- collected tags

Changes in v4:
- none

Changes in v3:
- none, this patch is new

 drivers/pci/controller/pcie-rzg3s-host.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index 6acb0231a067..c4d552f13613 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -1131,11 +1131,6 @@ static int rzg3s_pcie_set_max_link_speed(struct rzg3s_pcie_host *host)
 	    remote_supported_link_speeds != max_supported_link_speeds)
 		return 0;
 
-	/* Set target Link speed */
-	rzg3s_pcie_update_bits(host->pcie, pcie_cap + PCI_EXP_LNKCTL2,
-			       PCI_EXP_LNKCTL2_TLS,
-			       FIELD_PREP(PCI_EXP_LNKCTL2_TLS, link_speed));
-
 	/* Request link speed change */
 	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_PCCTRL2,
 			       RZG3S_PCI_PCCTRL2_LS_CHG_REQ |
-- 
2.43.0

[PATCH v7 09/14] PCI: rzg3s-host: Move host configuration code together

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Move host configuration code together to have it grouped. This prepares for
the addition of hotplug support.

Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v7:
- none

Changes in v6:
- none

Changes in v5:
- collected tags

Changes in v4:
- none

Changes in v3:
- none, this patch is new

 drivers/pci/controller/pcie-rzg3s-host.c | 248 +++++++++++------------
 1 file changed, 124 insertions(+), 124 deletions(-)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index c4d552f13613..b7035522ddfc 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -1343,130 +1343,6 @@ static int rzg3s_pcie_resets_prepare_and_get(struct rzg3s_pcie_host *host)
 							      host->cfg_resets);
 }
 
-static int rzg3s_pcie_host_parse_port(struct rzg3s_pcie_host *host)
-{
-	struct device_node *of_port __free(device_node) =
-		of_get_next_child(host->dev->of_node, NULL);
-	struct rzg3s_pcie_port *port = &host->port;
-	int ret;
-
-	ret = of_property_read_u32(of_port, "vendor-id", &port->vendor_id);
-	if (ret)
-		return ret;
-
-	ret = of_property_read_u32(of_port, "device-id", &port->device_id);
-	if (ret)
-		return ret;
-
-	port->refclk = of_clk_get_by_name(of_port, "ref");
-	if (IS_ERR(port->refclk))
-		return PTR_ERR(port->refclk);
-
-	return 0;
-}
-
-static int rzg3s_pcie_host_init_port(struct rzg3s_pcie_host *host)
-{
-	struct rzg3s_pcie_port *port = &host->port;
-	struct device *dev = host->dev;
-	int ret;
-
-	/* Enable access control to the CFGU */
-	writel_relaxed(RZG3S_PCI_PERM_CFG_HWINIT_EN,
-		       host->axi + RZG3S_PCI_PERM);
-
-	/* Update vendor ID and device ID */
-	writew_relaxed(port->vendor_id, host->pcie + PCI_VENDOR_ID);
-	writew_relaxed(port->device_id, host->pcie + PCI_DEVICE_ID);
-
-	/* Disable access control to the CFGU */
-	writel_relaxed(0, host->axi + RZG3S_PCI_PERM);
-
-	ret = clk_prepare_enable(port->refclk);
-	if (ret)
-		return dev_err_probe(dev, ret, "Failed to enable refclk!\n");
-
-	/* Set the PHY, if any */
-	if (host->data->init_phy) {
-		ret = host->data->init_phy(host);
-		if (ret) {
-			dev_err_probe(dev, ret, "Failed to set the PHY!\n");
-			goto refclk_disable;
-		}
-	}
-
-	return 0;
-
-refclk_disable:
-	clk_disable_unprepare(port->refclk);
-	return ret;
-}
-
-static int
-rzg3s_pcie_host_init(struct rzg3s_pcie_host *host,
-		     int (*init_irqdomain)(struct rzg3s_pcie_host *host),
-		     void (*teardown_irqdomain)(struct rzg3s_pcie_host *host))
-{
-	u32 val;
-	int ret;
-
-	/* SoC-specific pre-configuration */
-	if (host->data->config_pre_init)
-		host->data->config_pre_init(host);
-
-	/* Initialize the PCIe related registers */
-	ret = rzg3s_pcie_config_init(host);
-	if (ret)
-		goto config_deinit;
-
-	ret = rzg3s_pcie_host_init_port(host);
-	if (ret)
-		goto config_deinit;
-
-	/* Enable ASPM L1 transition for SoCs that use it */
-	ret = rzg3s_sysc_config_func(host->sysc,
-				     RZG3S_SYSC_FUNC_ID_L1_ALLOW, 1);
-	if (ret)
-		goto config_deinit_and_refclk;
-
-	/* Initialize the interrupts */
-	rzg3s_pcie_irq_init(host);
-
-	ret = init_irqdomain(host);
-	if (ret)
-		goto config_deinit_and_refclk;
-
-	/* SoC-specific post-configuration */
-	ret = host->data->config_post_init(host);
-	if (ret)
-		goto teardown_irqdomain;
-
-	/* Wait for link up */
-	ret = readl_poll_timeout(host->axi + RZG3S_PCI_PCSTAT1, val,
-				 !(val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS),
-				 PCIE_LINK_WAIT_SLEEP_MS * MILLI,
-				 PCIE_LINK_WAIT_SLEEP_MS * MILLI *
-				 PCIE_LINK_WAIT_MAX_RETRIES);
-	if (ret)
-		goto config_deinit_post;
-
-	val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT2);
-	dev_info(host->dev, "PCIe link status [0x%x]\n", val);
-
-	return 0;
-
-config_deinit_post:
-	host->data->config_deinit(host);
-teardown_irqdomain:
-	teardown_irqdomain(host);
-config_deinit_and_refclk:
-	clk_disable_unprepare(host->port.refclk);
-config_deinit:
-	if (host->data->config_pre_init)
-		host->data->config_deinit(host);
-	return ret;
-}
-
 static void rzg3s_pcie_set_inbound_window(struct rzg3s_pcie_host *host,
 					  u64 cpu_addr, u64 pci_addr, u64 size,
 					  int id)
@@ -1694,6 +1570,130 @@ static int rzg3s_soc_pcie_init_phy(struct rzg3s_pcie_host *host)
 	return 0;
 }
 
+static int rzg3s_pcie_host_parse_port(struct rzg3s_pcie_host *host)
+{
+	struct device_node *of_port __free(device_node) =
+		of_get_next_child(host->dev->of_node, NULL);
+	struct rzg3s_pcie_port *port = &host->port;
+	int ret;
+
+	ret = of_property_read_u32(of_port, "vendor-id", &port->vendor_id);
+	if (ret)
+		return ret;
+
+	ret = of_property_read_u32(of_port, "device-id", &port->device_id);
+	if (ret)
+		return ret;
+
+	port->refclk = of_clk_get_by_name(of_port, "ref");
+	if (IS_ERR(port->refclk))
+		return PTR_ERR(port->refclk);
+
+	return 0;
+}
+
+static int rzg3s_pcie_host_init_port(struct rzg3s_pcie_host *host)
+{
+	struct rzg3s_pcie_port *port = &host->port;
+	struct device *dev = host->dev;
+	int ret;
+
+	/* Enable access control to the CFGU */
+	writel_relaxed(RZG3S_PCI_PERM_CFG_HWINIT_EN,
+		       host->axi + RZG3S_PCI_PERM);
+
+	/* Update vendor ID and device ID */
+	writew_relaxed(port->vendor_id, host->pcie + PCI_VENDOR_ID);
+	writew_relaxed(port->device_id, host->pcie + PCI_DEVICE_ID);
+
+	/* Disable access control to the CFGU */
+	writel_relaxed(0, host->axi + RZG3S_PCI_PERM);
+
+	ret = clk_prepare_enable(port->refclk);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable refclk!\n");
+
+	/* Set the PHY, if any */
+	if (host->data->init_phy) {
+		ret = host->data->init_phy(host);
+		if (ret) {
+			dev_err_probe(dev, ret, "Failed to set the PHY!\n");
+			goto refclk_disable;
+		}
+	}
+
+	return 0;
+
+refclk_disable:
+	clk_disable_unprepare(port->refclk);
+	return ret;
+}
+
+static int
+rzg3s_pcie_host_init(struct rzg3s_pcie_host *host,
+		     int (*init_irqdomain)(struct rzg3s_pcie_host *host),
+		     void (*teardown_irqdomain)(struct rzg3s_pcie_host *host))
+{
+	u32 val;
+	int ret;
+
+	/* SoC-specific pre-configuration */
+	if (host->data->config_pre_init)
+		host->data->config_pre_init(host);
+
+	/* Initialize the PCIe related registers */
+	ret = rzg3s_pcie_config_init(host);
+	if (ret)
+		goto config_deinit;
+
+	ret = rzg3s_pcie_host_init_port(host);
+	if (ret)
+		goto config_deinit;
+
+	/* Enable ASPM L1 transition for SoCs that use it */
+	ret = rzg3s_sysc_config_func(host->sysc,
+				     RZG3S_SYSC_FUNC_ID_L1_ALLOW, 1);
+	if (ret)
+		goto config_deinit_and_refclk;
+
+	/* Initialize the interrupts */
+	rzg3s_pcie_irq_init(host);
+
+	ret = init_irqdomain(host);
+	if (ret)
+		goto config_deinit_and_refclk;
+
+	/* SoC-specific post-configuration */
+	ret = host->data->config_post_init(host);
+	if (ret)
+		goto teardown_irqdomain;
+
+	/* Wait for link up */
+	ret = readl_poll_timeout(host->axi + RZG3S_PCI_PCSTAT1, val,
+				 !(val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS),
+				 PCIE_LINK_WAIT_SLEEP_MS * MILLI,
+				 PCIE_LINK_WAIT_SLEEP_MS * MILLI *
+				 PCIE_LINK_WAIT_MAX_RETRIES);
+	if (ret)
+		goto config_deinit_post;
+
+	val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT2);
+	dev_info(host->dev, "PCIe link status [0x%x]\n", val);
+
+	return 0;
+
+config_deinit_post:
+	host->data->config_deinit(host);
+teardown_irqdomain:
+	teardown_irqdomain(host);
+config_deinit_and_refclk:
+	clk_disable_unprepare(host->port.refclk);
+config_deinit:
+	if (host->data->config_pre_init)
+		host->data->config_deinit(host);
+	return ret;
+}
+
 static int
 rzg3s_pcie_host_setup(struct rzg3s_pcie_host *host,
 		      int (*init_irqdomain)(struct rzg3s_pcie_host *host),
-- 
2.43.0

[PATCH v7 10/14] PCI: rzg3s-host: Move suspend/resume code into dedicated functions

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

In preparation for implementing hotplug using
pci_host_bridge::reset_root_port(), move the suspend/resume code into
rzg3s_pcie_host_stop() and rzg3s_pcie_host_start(). These functions
will later be reused by the hotplug implementation through
pci_host_bridge::reset_root_port().

Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v7:
- none

Changes in v6:
- none

Changes in v5:
- collected tags

Changes in v4:
- none

Changes in v3:
- none, this patch is new

 drivers/pci/controller/pcie-rzg3s-host.c | 181 ++++++++++++-----------
 1 file changed, 96 insertions(+), 85 deletions(-)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index b7035522ddfc..54d1fd200f35 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -1728,6 +1728,100 @@ rzg3s_pcie_host_setup(struct rzg3s_pcie_host *host,
 	return 0;
 }
 
+static int rzg3s_pcie_host_stop(struct rzg3s_pcie_host *host)
+{
+	const struct rzg3s_pcie_soc_data *data = host->data;
+	struct rzg3s_pcie_port *port = &host->port;
+	struct rzg3s_sysc *sysc = host->sysc;
+	int ret;
+
+	clk_disable_unprepare(port->refclk);
+
+	/* SoC-specific de-initialization */
+	ret = data->config_deinit(host);
+	if (ret)
+		goto refclk_restore;
+
+	ret = reset_control_bulk_assert(data->num_power_resets,
+					host->power_resets);
+	if (ret)
+		goto config_reinit;
+
+	/*
+	 * Since the power domain's genpd_suspend_noirq() will disable clocks,
+	 * there is no need to manually invoke runtime PM API here.
+	 */
+
+	ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 0);
+	if (ret)
+		goto power_resets_restore;
+
+	return 0;
+
+	/* Restore the previous state if any error happens */
+power_resets_restore:
+	reset_control_bulk_deassert(data->num_power_resets,
+				    host->power_resets);
+config_reinit:
+	if (data->config_pre_init)
+		data->config_pre_init(host);
+	data->config_post_init(host);
+refclk_restore:
+	clk_prepare_enable(port->refclk);
+	return ret;
+}
+
+static int rzg3s_pcie_host_start(struct rzg3s_pcie_host *host)
+{
+	const struct rzg3s_pcie_soc_data *data = host->data;
+	struct rzg3s_sysc *sysc = host->sysc;
+	int ret;
+
+	ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_MODE, 1);
+	if (ret)
+		return ret;
+
+	ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 1);
+	if (ret)
+		return ret;
+
+	if (host->num_lanes) {
+		ret = rzg3s_sysc_config_func(host->sysc,
+					     RZG3S_SYSC_FUNC_ID_LINK_MASTER,
+					     host->num_lanes == 2  ?
+					     RZG3S_SYSC_LINK_MODE_DUAL_X2 :
+					     RZG3S_SYSC_LINK_MODE_SINGLE_X4);
+		if (ret)
+			goto assert_rst_rsm_b;
+	}
+
+	/*
+	 * Since the power domain's genpd_resume_noirq() will enable clocks,
+	 * there is no need to manually invoke runtime PM API here.
+	 */
+
+	ret = rzg3s_pcie_power_resets_deassert(host);
+	if (ret)
+		goto assert_rst_rsm_b;
+
+	ret = rzg3s_pcie_host_setup(host, rzg3s_pcie_msi_hw_setup,
+				    rzg3s_pcie_msi_hw_teardown);
+	if (ret)
+		goto assert_power_resets;
+
+	return 0;
+
+	/*
+	 * If any error happens there is no way to recover the IP. Put it in the
+	 * lowest possible power state.
+	 */
+assert_power_resets:
+	reset_control_bulk_assert(data->num_power_resets, host->power_resets);
+assert_rst_rsm_b:
+	rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 0);
+	return ret;
+}
+
 static int rzg3s_pcie_get_controller_id(struct rzg3s_pcie_host *host)
 {
 	struct device_node *np = host->dev->of_node;
@@ -1931,98 +2025,15 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
 static int rzg3s_pcie_suspend_noirq(struct device *dev)
 {
 	struct rzg3s_pcie_host *host = dev_get_drvdata(dev);
-	const struct rzg3s_pcie_soc_data *data = host->data;
-	struct rzg3s_pcie_port *port = &host->port;
-	struct rzg3s_sysc *sysc = host->sysc;
-	int ret;
-
-	clk_disable_unprepare(port->refclk);
-
-	/* SoC-specific de-initialization */
-	ret = data->config_deinit(host);
-	if (ret)
-		goto refclk_restore;
-
-	ret = reset_control_bulk_assert(data->num_power_resets,
-					host->power_resets);
-	if (ret)
-		goto config_reinit;
-
-	/*
-	 * Since the power domain's genpd_suspend_noirq() will disable clocks,
-	 * there is no need to manually invoke runtime PM API here.
-	 */
-
-	ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 0);
-	if (ret)
-		goto power_resets_restore;
 
-	return 0;
-
-	/* Restore the previous state if any error happens */
-power_resets_restore:
-	reset_control_bulk_deassert(data->num_power_resets,
-				    host->power_resets);
-config_reinit:
-	if (data->config_pre_init)
-		data->config_pre_init(host);
-	data->config_post_init(host);
-refclk_restore:
-	clk_prepare_enable(port->refclk);
-	return ret;
+	return rzg3s_pcie_host_stop(host);
 }
 
 static int rzg3s_pcie_resume_noirq(struct device *dev)
 {
 	struct rzg3s_pcie_host *host = dev_get_drvdata(dev);
-	const struct rzg3s_pcie_soc_data *data = host->data;
-	struct rzg3s_sysc *sysc = host->sysc;
-	int ret;
 
-	ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_MODE, 1);
-	if (ret)
-		return ret;
-
-	ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 1);
-	if (ret)
-		return ret;
-
-	if (host->num_lanes) {
-		ret = rzg3s_sysc_config_func(host->sysc,
-					     RZG3S_SYSC_FUNC_ID_LINK_MASTER,
-					     host->num_lanes == 2  ?
-					     RZG3S_SYSC_LINK_MODE_DUAL_X2 :
-					     RZG3S_SYSC_LINK_MODE_SINGLE_X4);
-		if (ret)
-			goto assert_rst_rsm_b;
-	}
-
-	/*
-	 * Since the power domain's genpd_resume_noirq() will enable clocks,
-	 * there is no need to manually invoke runtime PM API here.
-	 */
-
-	ret = rzg3s_pcie_power_resets_deassert(host);
-	if (ret)
-		goto assert_rst_rsm_b;
-
-	ret = rzg3s_pcie_host_setup(host, rzg3s_pcie_msi_hw_setup,
-				    rzg3s_pcie_msi_hw_teardown);
-	if (ret)
-		goto assert_power_resets;
-
-	return 0;
-
-	/*
-	 * If any error happens there is no way to recover the IP. Put it in the
-	 * lowest possible power state.
-	 */
-assert_power_resets:
-	reset_control_bulk_assert(data->num_power_resets,
-				  host->power_resets);
-assert_rst_rsm_b:
-	rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 0);
-	return ret;
+	return rzg3s_pcie_host_start(host);
 }
 
 static const struct dev_pm_ops rzg3s_pcie_pm_ops = {
-- 
2.43.0

[PATCH v7 11/14] PCI: rzg3s-host: Move IRQ domain setup code

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Subsequent patches add support for the event IRQ to handle link
up/down events. The event IRQ handler will use
rzg3s_pcie_set_max_link_speed(). In preparation for adding event IRQ
support, move the IRQ domain initialization code after
rzg3s_pcie_set_max_link_speed().

Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v7:
- none

Changes in v6:
- none

Changes in v5:
- collected tags
- restored the previous order in rzg3s_pcie_teardown_intx()

Changes in v4:
- none

Changes in v3:
- none, this patch is new

 drivers/pci/controller/pcie-rzg3s-host.c | 143 +++++++++++------------
 1 file changed, 71 insertions(+), 72 deletions(-)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index 54d1fd200f35..e104c6b4ae4a 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -995,78 +995,6 @@ static const struct irq_domain_ops rzg3s_pcie_intx_domain_ops = {
 	.xlate = irq_domain_xlate_onetwocell,
 };
 
-static void rzg3s_pcie_teardown_intx(struct rzg3s_pcie_host *host,
-				     int count)
-{
-	while (--count >= 0) {
-		irq_set_chained_handler_and_data(host->intx_irqs[count], NULL,
-						 NULL);
-	}
-
-	if (host->intx_domain)
-		irq_domain_remove(host->intx_domain);
-}
-
-static int rzg3s_pcie_init_irqdomain(struct rzg3s_pcie_host *host)
-{
-	struct device *dev = host->dev;
-	struct platform_device *pdev = to_platform_device(dev);
-	int i, ret;
-
-	host->intx_domain = irq_domain_create_linear(dev_fwnode(dev),
-						     PCI_NUM_INTX,
-						     &rzg3s_pcie_intx_domain_ops,
-						     host);
-	if (!host->intx_domain) {
-		return dev_err_probe(dev, -EINVAL,
-				     "Failed to add irq domain for INTx IRQs\n");
-	}
-	irq_domain_update_bus_token(host->intx_domain, DOMAIN_BUS_WIRED);
-
-	for (i = 0; i < PCI_NUM_INTX; i++) {
-		char irq_name[5] = {0};
-		int irq;
-
-		scnprintf(irq_name, ARRAY_SIZE(irq_name), "int%c", 'a' + i);
-
-		irq = platform_get_irq_byname(pdev, irq_name);
-		if (irq < 0) {
-			ret = irq;
-			dev_err_probe(dev, ret,
-				      "Failed to parse and map INT%c IRQ\n",
-				      'A' + i);
-			goto teardown_intx;
-		}
-
-		host->intx_irqs[i] = irq;
-		irq_set_chained_handler_and_data(irq,
-						 rzg3s_pcie_intx_irq_handler,
-						 host);
-	}
-
-	if (IS_ENABLED(CONFIG_PCI_MSI)) {
-		ret = rzg3s_pcie_init_msi(host);
-
-		if (ret)
-			goto teardown_intx;
-	}
-
-	return 0;
-
-teardown_intx:
-	rzg3s_pcie_teardown_intx(host, i);
-
-	return ret;
-}
-
-static void rzg3s_pcie_teardown_irqdomain(struct rzg3s_pcie_host *host)
-{
-	if (IS_ENABLED(CONFIG_PCI_MSI))
-		rzg3s_pcie_teardown_msi(host);
-
-	rzg3s_pcie_teardown_intx(host, PCI_NUM_INTX);
-}
-
 static int rzg3s_pcie_set_max_link_speed(struct rzg3s_pcie_host *host)
 {
 	u32 remote_supported_link_speeds, max_supported_link_speeds;
@@ -1156,6 +1084,77 @@ static int rzg3s_pcie_set_max_link_speed(struct rzg3s_pcie_host *host)
 	return ret;
 }
 
+static void rzg3s_pcie_teardown_intx(struct rzg3s_pcie_host *host, int count)
+{
+	while (--count >= 0) {
+		irq_set_chained_handler_and_data(host->intx_irqs[count], NULL,
+						 NULL);
+	}
+
+	if (host->intx_domain)
+		irq_domain_remove(host->intx_domain);
+}
+
+static int rzg3s_pcie_init_irqdomain(struct rzg3s_pcie_host *host)
+{
+	struct device *dev = host->dev;
+	struct platform_device *pdev = to_platform_device(dev);
+	int i, ret;
+
+	host->intx_domain = irq_domain_create_linear(dev_fwnode(dev),
+						     PCI_NUM_INTX,
+						     &rzg3s_pcie_intx_domain_ops,
+						     host);
+	if (!host->intx_domain) {
+		return dev_err_probe(dev, -EINVAL,
+				     "Failed to add irq domain for INTx IRQs\n");
+	}
+	irq_domain_update_bus_token(host->intx_domain, DOMAIN_BUS_WIRED);
+
+	for (i = 0; i < PCI_NUM_INTX; i++) {
+		char irq_name[5] = {0};
+		int irq;
+
+		scnprintf(irq_name, ARRAY_SIZE(irq_name), "int%c", 'a' + i);
+
+		irq = platform_get_irq_byname(pdev, irq_name);
+		if (irq < 0) {
+			ret = irq;
+			dev_err_probe(dev, ret,
+				      "Failed to parse and map INT%c IRQ\n",
+				      'A' + i);
+			goto teardown_intx;
+		}
+
+		host->intx_irqs[i] = irq;
+		irq_set_chained_handler_and_data(irq,
+						 rzg3s_pcie_intx_irq_handler,
+						 host);
+	}
+
+	if (IS_ENABLED(CONFIG_PCI_MSI)) {
+		ret = rzg3s_pcie_init_msi(host);
+
+		if (ret)
+			goto teardown_intx;
+	}
+
+	return 0;
+
+teardown_intx:
+	rzg3s_pcie_teardown_intx(host, i);
+
+	return ret;
+}
+
+static void rzg3s_pcie_teardown_irqdomain(struct rzg3s_pcie_host *host)
+{
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		rzg3s_pcie_teardown_msi(host);
+
+	rzg3s_pcie_teardown_intx(host, PCI_NUM_INTX);
+}
+
 static int rzg3s_pcie_config_init(struct rzg3s_pcie_host *host)
 {
 	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(host);
-- 
2.43.0

[PATCH v7 12/14] PCI: rzg3s-host: Add helper to wait for link up

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Factor out the link up polling from rzg3s_pcie_host_init() into a new
helper. With the link up/down event handling added by a subsequent
commit, the probe will no longer fail when the link is down, but
continue and rely on later link up events to detect the link becoming
active. Having the polling in a dedicated helper allows the probe path
to treat its failure as non-fatal.

No functional change.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v7:
- none

Changes in v6:
- none, this patch is new

 drivers/pci/controller/pcie-rzg3s-host.c | 30 ++++++++++++++++--------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index e104c6b4ae4a..1ced6d83c7c1 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -1084,6 +1084,25 @@ static int rzg3s_pcie_set_max_link_speed(struct rzg3s_pcie_host *host)
 	return ret;
 }
 
+static int rzg3s_pcie_wait_for_link_up(struct rzg3s_pcie_host *host)
+{
+	u32 val;
+	int ret;
+
+	ret = readl_poll_timeout(host->axi + RZG3S_PCI_PCSTAT1, val,
+				 !(val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS),
+				 PCIE_LINK_WAIT_SLEEP_MS * MILLI,
+				 PCIE_LINK_WAIT_SLEEP_MS * MILLI *
+				 PCIE_LINK_WAIT_MAX_RETRIES);
+	if (ret)
+		return ret;
+
+	val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT2);
+	dev_info(host->dev, "PCIe link status [0x%x]\n", val);
+
+	return 0;
+}
+
 static void rzg3s_pcie_teardown_intx(struct rzg3s_pcie_host *host, int count)
 {
 	while (--count >= 0) {
@@ -1633,7 +1652,6 @@ rzg3s_pcie_host_init(struct rzg3s_pcie_host *host,
 		     int (*init_irqdomain)(struct rzg3s_pcie_host *host),
 		     void (*teardown_irqdomain)(struct rzg3s_pcie_host *host))
 {
-	u32 val;
 	int ret;
 
 	/* SoC-specific pre-configuration */
@@ -1667,18 +1685,10 @@ rzg3s_pcie_host_init(struct rzg3s_pcie_host *host,
 	if (ret)
 		goto teardown_irqdomain;
 
-	/* Wait for link up */
-	ret = readl_poll_timeout(host->axi + RZG3S_PCI_PCSTAT1, val,
-				 !(val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS),
-				 PCIE_LINK_WAIT_SLEEP_MS * MILLI,
-				 PCIE_LINK_WAIT_SLEEP_MS * MILLI *
-				 PCIE_LINK_WAIT_MAX_RETRIES);
+	ret = rzg3s_pcie_wait_for_link_up(host);
 	if (ret)
 		goto config_deinit_post;
 
-	val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT2);
-	dev_info(host->dev, "PCIe link status [0x%x]\n", val);
-
 	return 0;
 
 config_deinit_post:
-- 
2.43.0

[PATCH v7 13/14] PCI: rzg3s-host: Re-enumerate the bus on PCIe link-state changes

[Claudiu Beznea]

From: John Madieu <john.madieu.xa@bp.renesas.com>

The RZ/G3{E, S}, RZ/V2{H(P), N} PCIe controllers do not expose the
standard PCIe Slot Capability registers, so the generic pciehp driver
cannot be used. The only link-state signal the hardware provides is the
DL_UpDown bit in the PEIS0 event status register, which is raised on every
Data Link layer up/down transition.

Enable DL_UpDown in PEIE0 and hook up an interrupt handler so the driver
can react to link-state changes: a device that trains after boot gets
enumerated, and a device that disappears on link loss is removed. This
provides hotplug-like behavior without the PCI hotplug core, which is
unavailable for the reason above.

On a DL_UpDown event the handler acks the W1C status bit and inspects
PCSTAT1.DL_DOWN_STS:

- Link up: re-run max link speed negotiation, wait for the link to settle
  and rescan the root bus. To cover the case where the device is replaced
  or removed while the system is suspended add the
  rzg3s_pcie_device_replaced(). It reads the vendor ID, device ID,
  revision ID and class code of the device behind the root port and
  compares them against the values cached in the corresponding
  struct pci_dev, along with the Device Serial Number cached in
  struct rzg3s_pcie_host::con_dev_dsn, mirroring pciehp_device_replaced().
  If the device is detected as replaced (or no longer present),
  pci_dev_set_disconnected() is called for all the devices on the bus
  below the root port, so that the teardown of the old devices does not
  end up accessing the newly connected device, and the devices are then
  removed. If a link up/down cycle occurred while devices are on the bus
  (fast link transitions coalesced into a single DL_UpDown event), the
  devices are removed and re-enumerated even if the connected device did
  not change.
- Link down: walk the bus in reverse and pci_stop_and_remove_bus_device()
  each child.

Both paths take pci_lock_rescan_remove() to serialize against the PCI
core.

Link events are processed only after the controller has been fully
initialized.

Add rzg3s_pcie_pm_notifier() to avoid the link event interrupt interfering
with the suspend/resume of the other PCIe devices in the topology, and to
avoid deadlocks caused by pci_dev_lock() being taken from the link event
handler while suspend/resume is in progress. The notifier disables the link
event interrupt before suspend and enables it and rescans the bus, on
resume, to pick up devices that may have been plugged or unplugged while
the system was suspended. struct rzg3s_pcie_host::started was added to
avoid touching the controller registers in case the resume failed.

While at it, make probe tolerant of an absent device. Previously, if the
link failed to come up during rzg3s_pcie_host_setup(), probe tore the
controller back down and failed. Ignore the timeout returned by
rzg3s_pcie_wait_for_link_up(), leave the controller and refclk running,
and let the link-up path enumerate the device once it appears.

Signed-off-by: John Madieu <john.madieu.xa@bp.renesas.com>
Co-developed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v7:
- in rzg3s_pcie_remove_devices() set the device and the subordinate ones
  as disconnected before stop and remove them
- in rzg3s_pcie_event_irq_thread() kept the line 80 chars aligned
- in rzg3s_pcie_pm_notifier():
-- suspend path: disable the link IRQ at controller level as well
   (prerequisite for the next patch)
-- resume path: keep the sequence of enabling the link IRQ same as
   in probe
- due to this, dropped Prabakar's Rb tag

Changes in v6:
- moved the struct rzg3s_pcie_host::started in this patch to avoid
  setting the controller in failure cases (started = false)
- adopted the approch from pciehp driver with regards to checking the
  DSN of the connected device and decide, based on this, if the bus
  devices need to be removed + rescanned; the DSN of the connected
  device is cached in probe and link up events
- in case of link up events with !list_empty(&bus->devices) remove
  first the devices from the bus and re-scan (adopted from pciehp driver)
- in rzg3s_pcie_pm_notifier() touch the HW registers only if host->started
- in rzg3s_pcie_resume_noirq() call pci_dev_set_disconnected() for the
  devices on the bus if the connected device is changed at the resume
  similar to what the pciehp driver is doing
- made adjustments to rzg3s_pcie_wait_for_link_up()
- dropped the tags
- adjusted patch description

Changes in v5:
- in rzg3s_pcie_link_event(), on link up path, added pcie_bus_configure_settings();
  and dropped pci_rescan_bus() and inlined its instructions
- pm_runtime_get_sync(&bridge->dev)/pm_runtime_put_sync(&bridge->dev) to avoid
  "pci 0000:00:00.0: runtime PM trying to activate child device 0000:00:00.0 but
  parent (pci0000:00) is not active"
- added a PM notifier and disable/enable the link interrupt
  before suspend/after resume to avoid deadlocks b/w PM core and any PCI
  core code calling pci_dev_lock(); the next patch uses pci_host_handle_link_down()
  which calls pci_dev_lock() on the following path:
  pci_host_handle_link_down() -> 
    pci_host_reset_root_port() ->
      pci_bus_error_reset() ->
        pci_reset_bridge() ->
          pci_slot_reset() ->
            pci_slot_lock() ->
              __pci_bus_lock() ->
                pci_dev_lock()
- added struct rzg3s_pcie_host::link_rescan to force a link rescan on return
  from resume and handle scenarios where devices dissaper while in suspend
- updated the commit message to reflect this
- didn't collect the tags from v4 dues to these changes

Changes in v4:
- dropped .reset_root_port() changes

Changes in v3:
- added RZG3S_PCI_PEIE0_DL_UPDOWN
- re-worked the support by implemeting
  struct pci_host_bridge::reset_root_port()
- introduced the struct rzg3s_pcie_host::state to:
-- avoid touching the controller while a reset root port is in progress
-- and avoid touching the controller in case a reset root port failed
-- and to be able to re-use the already existing code in the reset
   root port function
-- and added CLASS() constructs helpers for it to keep the state handling
   code simpler
- updated the patch description to reflect the updates

 drivers/pci/controller/pcie-rzg3s-host.c | 321 +++++++++++++++++++++--
 1 file changed, 303 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index 1ced6d83c7c1..a1c9e29f43e5 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -33,6 +33,7 @@
 #include <linux/reset.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
+#include <linux/suspend.h>
 #include <linux/units.h>
 
 #include "pci-host-common.h"
@@ -87,6 +88,7 @@
 #define RZG3S_PCI_MSGRCVIS_MRI			BIT(24)
 
 #define RZG3S_PCI_PEIE0				0x200
+#define RZG3S_PCI_PEIE0_DL_UPDOWN		BIT(9)
 
 #define RZG3S_PCI_PEIS0				0x204
 #define RZG3S_PCI_PEIS0_RX_DLLP_PM_ENTER	BIT(12)
@@ -324,9 +326,16 @@ struct rzg3s_pcie_port {
  * @msi: MSI data structure
  * @port: PCIe Root Port
  * @hw_lock: lock for access to the HW resources
+ * @pm_nb: PM notifier block (for link events)
+ * @con_dev_dsn: Device Serial Number for the connected device, used to
+ * determine whether a hotplugged device was replaced with a different one
+ * during system sleep
+ * @event_irq: PCIe event interrupt for DL_UpDown detection
  * @intx_irqs: INTx interrupts
  * @max_link_speed: maximum supported link speed
  * @controller_id: PCIe controller identifier, used for System Controller access
+ * @link_rescan: The PCIe link rescan state
+ * @started: The PCIe controller state (started or not)
  * @num_lanes: The number of lanes
  */
 struct rzg3s_pcie_host {
@@ -341,9 +350,14 @@ struct rzg3s_pcie_host {
 	struct rzg3s_pcie_msi msi;
 	struct rzg3s_pcie_port port;
 	raw_spinlock_t hw_lock;
+	struct notifier_block pm_nb;
+	u64 con_dev_dsn;
+	int event_irq;
 	int intx_irqs[PCI_NUM_INTX];
 	int max_link_speed;
 	enum rzg3s_pcie_controller_id controller_id;
+	bool link_rescan;
+	bool started;
 	u8 num_lanes;
 };
 
@@ -1094,12 +1108,187 @@ static int rzg3s_pcie_wait_for_link_up(struct rzg3s_pcie_host *host)
 				 PCIE_LINK_WAIT_SLEEP_MS * MILLI,
 				 PCIE_LINK_WAIT_SLEEP_MS * MILLI *
 				 PCIE_LINK_WAIT_MAX_RETRIES);
-	if (ret)
-		return ret;
+	if (ret) {
+		dev_info(host->dev,
+			 "PCIe link down, waiting for DL_UpDown\n");
+	}
 
 	val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT2);
 	dev_info(host->dev, "PCIe link status [0x%x]\n", val);
 
+	return ret;
+}
+
+static struct pci_dev
+*rzg3s_pcie_get_connected_dev(struct rzg3s_pcie_host *host)
+{
+	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(host);
+	struct pci_dev *port __free(pci_dev_put) =
+		pci_get_slot(bridge->bus, PCI_DEVFN(0, 0));
+
+	if (!port || !port->subordinate)
+		return NULL;
+
+	return pci_get_slot(port->subordinate, PCI_DEVFN(0, 0));
+}
+
+static void rzg3s_pcie_cache_connected_dev_id(struct rzg3s_pcie_host *host)
+{
+	struct pci_dev *con_dev __free(pci_dev_put) =
+		rzg3s_pcie_get_connected_dev(host);
+
+	host->con_dev_dsn = con_dev ? pci_get_dsn(con_dev) : 0;
+}
+
+static bool rzg3s_pcie_device_replaced(struct rzg3s_pcie_host *host)
+{
+	struct pci_dev *con_dev __free(pci_dev_put) =
+		rzg3s_pcie_get_connected_dev(host);
+	u32 reg;
+
+	if (!con_dev)
+		return true;
+
+	if (pci_read_config_dword(con_dev, PCI_VENDOR_ID, &reg) ||
+	    reg != (con_dev->vendor | (con_dev->device << 16)) ||
+	    pci_read_config_dword(con_dev, PCI_CLASS_REVISION, &reg) ||
+	    reg != (con_dev->revision | (con_dev->class << 8)))
+		return true;
+
+	if (con_dev->hdr_type == PCI_HEADER_TYPE_NORMAL &&
+	    (pci_read_config_dword(con_dev, PCI_SUBSYSTEM_VENDOR_ID, &reg) ||
+	     reg != (con_dev->subsystem_vendor |
+		     (con_dev->subsystem_device << 16))))
+		return true;
+
+	if (pci_get_dsn(con_dev) != host->con_dev_dsn)
+		return true;
+
+	return false;
+}
+
+static void rzg3s_pcie_remove_devices(struct pci_bus *bus)
+{
+	struct pci_dev *dev, *tmp;
+
+	list_for_each_entry_safe_reverse(dev, tmp, &bus->devices, bus_list) {
+		pci_dev_set_disconnected(dev, NULL);
+		if (pci_has_subordinate(dev)) {
+			pci_walk_bus(dev->subordinate, pci_dev_set_disconnected,
+				     NULL);
+		}
+		pci_stop_and_remove_bus_device(dev);
+	}
+}
+
+static void rzg3s_pcie_link_event(struct rzg3s_pcie_host *host, bool bounced)
+{
+	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(host);
+	struct pci_bus *bus = bridge->bus, *child;
+	u32 val;
+	int ret;
+
+	pci_lock_rescan_remove();
+
+	val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT1);
+	if (val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS) {
+		if (!list_empty(&bus->devices)) {
+			dev_info(host->dev,
+				 "PCIe link down, removing devices\n");
+			rzg3s_pcie_remove_devices(bus);
+		}
+		goto unlock;
+	}
+
+	if (!list_empty(&bus->devices)) {
+		bool replaced = rzg3s_pcie_device_replaced(host);
+
+		/*
+		 * If bounced = false it means link rescanning (after probe
+		 * or system resume), no link transition was recorded. The
+		 * enumerated devices are intact, so remove them only if the
+		 * connected device was replaced while link events were not
+		 * delivered.
+		 *
+		 * If bounced = true it means at least one real link down/up
+		 * cycle occurred.
+		 */
+		if (!bounced && !replaced)
+			goto unlock;
+
+		rzg3s_pcie_remove_devices(bus);
+	}
+
+	dev_info(host->dev, "PCIe link up, rescanning bus\n");
+
+	ret = rzg3s_pcie_set_max_link_speed(host);
+	if (ret)
+		dev_info(host->dev, "Failed to set max link speed\n");
+
+	pci_host_common_link_train_delay(host->max_link_speed);
+
+	pm_runtime_get_sync(&bridge->dev);
+
+	pci_scan_child_bus(bus);
+	pci_assign_unassigned_bus_resources(bus);
+	list_for_each_entry(child, &bus->children, node)
+		pcie_bus_configure_settings(child);
+	pci_bus_add_devices(bus);
+
+	rzg3s_pcie_cache_connected_dev_id(host);
+
+	pm_runtime_put_sync(&bridge->dev);
+unlock:
+	pci_unlock_rescan_remove();
+}
+
+static irqreturn_t rzg3s_pcie_event_irq_thread(int irq, void *data)
+{
+	struct rzg3s_pcie_host *host = data;
+	bool bounced;
+	u32 status;
+
+	status = readl_relaxed(host->axi + RZG3S_PCI_PEIS0);
+	bounced = status & RZG3S_PCI_PEIS0_DL_UPDOWN;
+
+	if (!bounced && !READ_ONCE(host->link_rescan))
+		return IRQ_NONE;
+
+	/* Clear the DL_UpDown status (W1C) */
+	if (bounced) {
+		writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN,
+			       host->axi + RZG3S_PCI_PEIS0);
+	}
+	WRITE_ONCE(host->link_rescan, false);
+
+	rzg3s_pcie_link_event(host, bounced);
+
+	return IRQ_HANDLED;
+}
+
+static int rzg3s_pcie_request_event_irq(struct rzg3s_pcie_host *host)
+{
+	struct device *dev = host->dev;
+	struct platform_device *pdev = to_platform_device(dev);
+	const char *evt_name;
+	int ret, irq;
+
+	evt_name = devm_kasprintf(dev, GFP_KERNEL, "%s-evt", dev_name(dev));
+	if (!evt_name)
+		return -ENOMEM;
+
+	irq = platform_get_irq_byname(pdev, "pcie_evt");
+	if (irq < 0)
+		return irq;
+
+	ret = request_threaded_irq(irq, NULL, rzg3s_pcie_event_irq_thread,
+				   IRQF_ONESHOT, evt_name, host);
+	if (ret) {
+		return dev_err_probe(dev, ret,
+				     "Failed to request pcie_evt IRQ\n");
+	}
+	host->event_irq = irq;
+
 	return 0;
 }
 
@@ -1114,6 +1303,17 @@ static void rzg3s_pcie_teardown_intx(struct rzg3s_pcie_host *host, int count)
 		irq_domain_remove(host->intx_domain);
 }
 
+static void rzg3s_pcie_teardown_irqdomain(struct rzg3s_pcie_host *host)
+{
+	if (host->event_irq > 0)
+		free_irq(host->event_irq, host);
+
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		rzg3s_pcie_teardown_msi(host);
+
+	rzg3s_pcie_teardown_intx(host, PCI_NUM_INTX);
+}
+
 static int rzg3s_pcie_init_irqdomain(struct rzg3s_pcie_host *host)
 {
 	struct device *dev = host->dev;
@@ -1158,22 +1358,21 @@ static int rzg3s_pcie_init_irqdomain(struct rzg3s_pcie_host *host)
 			goto teardown_intx;
 	}
 
+	ret = rzg3s_pcie_request_event_irq(host);
+	if (ret)
+		goto teardown_msi;
+
 	return 0;
 
+teardown_msi:
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		rzg3s_pcie_teardown_msi(host);
 teardown_intx:
 	rzg3s_pcie_teardown_intx(host, i);
 
 	return ret;
 }
 
-static void rzg3s_pcie_teardown_irqdomain(struct rzg3s_pcie_host *host)
-{
-	if (IS_ENABLED(CONFIG_PCI_MSI))
-		rzg3s_pcie_teardown_msi(host);
-
-	rzg3s_pcie_teardown_intx(host, PCI_NUM_INTX);
-}
-
 static int rzg3s_pcie_config_init(struct rzg3s_pcie_host *host)
 {
 	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(host);
@@ -1685,14 +1884,8 @@ rzg3s_pcie_host_init(struct rzg3s_pcie_host *host,
 	if (ret)
 		goto teardown_irqdomain;
 
-	ret = rzg3s_pcie_wait_for_link_up(host);
-	if (ret)
-		goto config_deinit_post;
-
 	return 0;
 
-config_deinit_post:
-	host->data->config_deinit(host);
 teardown_irqdomain:
 	teardown_irqdomain(host);
 config_deinit_and_refclk:
@@ -1728,6 +1921,10 @@ rzg3s_pcie_host_setup(struct rzg3s_pcie_host *host,
 		return dev_err_probe(dev, ret,
 				     "Failed to initialize the HW!\n");
 
+	ret = rzg3s_pcie_wait_for_link_up(host);
+	if (ret)
+		return 0;
+
 	ret = rzg3s_pcie_set_max_link_speed(host);
 	if (ret)
 		dev_info(dev, "Failed to set max link speed\n");
@@ -1744,6 +1941,9 @@ static int rzg3s_pcie_host_stop(struct rzg3s_pcie_host *host)
 	struct rzg3s_sysc *sysc = host->sysc;
 	int ret;
 
+	if (!host->started)
+		return 0;
+
 	clk_disable_unprepare(port->refclk);
 
 	/* SoC-specific de-initialization */
@@ -1765,6 +1965,8 @@ static int rzg3s_pcie_host_stop(struct rzg3s_pcie_host *host)
 	if (ret)
 		goto power_resets_restore;
 
+	host->started = false;
+
 	return 0;
 
 	/* Restore the previous state if any error happens */
@@ -1786,6 +1988,9 @@ static int rzg3s_pcie_host_start(struct rzg3s_pcie_host *host)
 	struct rzg3s_sysc *sysc = host->sysc;
 	int ret;
 
+	if (host->started)
+		return 0;
+
 	ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_MODE, 1);
 	if (ret)
 		return ret;
@@ -1818,6 +2023,8 @@ static int rzg3s_pcie_host_start(struct rzg3s_pcie_host *host)
 	if (ret)
 		goto assert_power_resets;
 
+	host->started = true;
+
 	return 0;
 
 	/*
@@ -1901,6 +2108,44 @@ static void rzv2h_pcie_release_lanes(void *data)
 	rzv2h_num_total_lanes -= host->num_lanes;
 }
 
+static int rzg3s_pcie_pm_notifier(struct notifier_block *nb,
+				  unsigned long action, void *data)
+{
+	struct rzg3s_pcie_host *host = container_of(nb, struct rzg3s_pcie_host,
+						    pm_nb);
+
+	switch (action) {
+	case PM_SUSPEND_PREPARE:
+		/* Disable link up/down interrupts. */
+		if (host->started)
+			writel_relaxed(0, host->axi + RZG3S_PCI_PEIE0);
+		disable_irq(host->event_irq);
+		break;
+
+	case PM_POST_SUSPEND:
+		/* Enable link up/down interrupts and force link re-scan. */
+		if (host->started) {
+			/*
+			 * The link re-train in resume triggers the link up IRQ
+			 * if there are connected devices. Clear the DL_UpDown
+			 * status and trigger link_rescan to avoid
+			 * re-enumerating the already connected devices.
+			 */
+			writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN,
+				       host->axi + RZG3S_PCI_PEIS0);
+			WRITE_ONCE(host->link_rescan, true);
+			writel_relaxed(RZG3S_PCI_PEIE0_DL_UPDOWN,
+				       host->axi + RZG3S_PCI_PEIE0);
+		}
+		enable_irq(host->event_irq);
+		if (host->started)
+			irq_wake_thread(host->event_irq, host);
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
+
 static int rzg3s_pcie_probe(struct platform_device *pdev)
 {
 	struct pci_host_bridge *bridge;
@@ -1999,15 +2244,38 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		goto power_resets_assert;
 
+	host->started = true;
+	host->pm_nb.notifier_call = rzg3s_pcie_pm_notifier;
+	ret = register_pm_notifier(&host->pm_nb);
+	if (ret)
+		goto host_probe_teardown;
+
 	bridge->sysdata = host;
 	bridge->ops = &rzg3s_pcie_root_ops;
 	bridge->child_ops = &rzg3s_pcie_child_ops;
 	ret = pci_host_probe(bridge);
 	if (ret)
-		goto host_probe_teardown;
+		goto pm_notifier_unregister;
+
+	pci_lock_rescan_remove();
+	rzg3s_pcie_cache_connected_dev_id(host);
+	pci_unlock_rescan_remove();
+
+	/*
+	 * The link re-train triggers the link up IRQ if there are connected
+	 * devices. Clear DL_UpDown status and trigger link_rescan to avoid
+	 * re-enumerating the devices detected while pci_host_probe() and loose
+	 * any potential event.
+	 */
+	writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIS0);
+	WRITE_ONCE(host->link_rescan, true);
+	writel_relaxed(RZG3S_PCI_PEIE0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIE0);
+	irq_wake_thread(host->event_irq, host);
 
 	return 0;
 
+pm_notifier_unregister:
+	unregister_pm_notifier(&host->pm_nb);
 host_probe_teardown:
 	rzg3s_pcie_teardown_irqdomain(host);
 	clk_disable_unprepare(host->port.refclk);
@@ -2041,8 +2309,25 @@ static int rzg3s_pcie_suspend_noirq(struct device *dev)
 static int rzg3s_pcie_resume_noirq(struct device *dev)
 {
 	struct rzg3s_pcie_host *host = dev_get_drvdata(dev);
+	int ret;
 
-	return rzg3s_pcie_host_start(host);
+	ret = rzg3s_pcie_host_start(host);
+	if (ret)
+		return ret;
+
+	if (rzg3s_pcie_device_replaced(host)) {
+		struct pci_host_bridge *bridge =
+			pci_host_bridge_from_priv(host);
+		struct pci_dev *port __free(pci_dev_put) =
+			pci_get_slot(bridge->bus, PCI_DEVFN(0, 0));
+
+		if (port && port->subordinate) {
+			pci_walk_bus(port->subordinate,
+				     pci_dev_set_disconnected, NULL);
+		}
+	}
+
+	return 0;
 }
 
 static const struct dev_pm_ops rzg3s_pcie_pm_ops = {
-- 
2.43.0

[PATCH v7 14/14] PCI: rzg3s-host: Add bridge::reset_root_port()

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Add bridge::reset_root_port(). This allows, on some devices, performing
reads and writes to an NVMe endpoint after a link down/link up cycle
without failures.

The implementation of struct pci_host_bridge::reset_root_port() masks all
enabled interrupts and synchronizes them before resetting the controller
to prevent asynchronous events from interfering with the reset operation.

The struct rzg3s_pcie_host::resetting member is used to prevent the IRQ
setting calls done through the IRQ subsystem (mask, unmask) from
accessing the controller registers while .reset_root_port() has powered
down the IP. It also rejects a concurrent .reset_root_port() with -EBUSY.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v7:
- added struct rzg3s_pcie_host::resetting to avoid concurrency b/w IRQ
  subsystem and the .reset_root_port() and b/w .reset_root_port() and
  the link IRQ thread
- in rzg3s_pcie_link_event()
-- dropped the usage of __free(pci_dev_put)
-- don't continue if the .reset_root_port() filed to start the IP or
   if there is a reset in progress
- in rzg3s_pcie_event_irq_thread() don't continue if the reset is in
  progress
- access host->started with READ_ONCE()/WRITE_ONCE()
- use disable_irq() in .reset_root_port()
- in rzg3s_pcie_pm_notifier() checked host->resetting and host->started
  before accessing IP registers

Changes in v6:
- call pci_host_handle_link_down() if at least a link event occured
  and there are devices on the bus.
- dropped struct rzg3s_pcie_host::stared from here since it is now
  part of patch 8/9
- read the link IRQs in .reset_root_port() and restore them at the
  end
- flush the IRQ mask before synchronize()
- in .reset_root_port() clear the link IRQ before enabling it to cope
  with the new link handling procedure
- due to these dropped the tags

Changes in v5:
- lock around reads/writes on RZG3S_PCI_PINTRCVIE
- in rzg3s_pcie_host_reset_root_port() kept non-relaxed reads/writes
  on interrupt disable registers (even though the spin lock/unlock are around
  them) to emphasize the need for updates before the IRQs are synchronized and
  relaxed variants on interrupt enable path
- collected tags

Changes in v4:
- this patch is new and is a split from patch 7 of v3
- use a boolean to keep track of the controller start state; with this
  the state management code from patch 7 of v3 was dropped 


 drivers/pci/controller/Kconfig           |   1 +
 drivers/pci/controller/pcie-rzg3s-host.c | 184 ++++++++++++++++++++---
 2 files changed, 167 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 7e550c474b7a..c747bd5a9ec1 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -296,6 +296,7 @@ config PCIE_RENESAS_RZG3S_HOST
 	depends on ARCH_RENESAS || COMPILE_TEST
 	select MFD_SYSCON
 	select IRQ_MSI_LIB
+	select PCI_HOST_COMMON
 	help
 	  Say Y here if you want PCIe host controller support on Renesas RZ/G3S
 	  SoC.
diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index a1c9e29f43e5..f6823c9bc414 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -336,6 +336,7 @@ struct rzg3s_pcie_port {
  * @controller_id: PCIe controller identifier, used for System Controller access
  * @link_rescan: The PCIe link rescan state
  * @started: The PCIe controller state (started or not)
+ * @resetting: Mentions if the root port reset is in progress
  * @num_lanes: The number of lanes
  */
 struct rzg3s_pcie_host {
@@ -358,6 +359,7 @@ struct rzg3s_pcie_host {
 	enum rzg3s_pcie_controller_id controller_id;
 	bool link_rescan;
 	bool started;
+	bool resetting;
 	u8 num_lanes;
 };
 
@@ -653,6 +655,9 @@ static void rzg3s_pcie_msi_irq_mask(struct irq_data *d)
 
 	guard(raw_spinlock_irqsave)(&host->hw_lock);
 
+	if (host->resetting)
+		return;
+
 	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_MSIRM(reg_id), BIT(reg_bit),
 			       BIT(reg_bit));
 }
@@ -666,6 +671,9 @@ static void rzg3s_pcie_msi_irq_unmask(struct irq_data *d)
 
 	guard(raw_spinlock_irqsave)(&host->hw_lock);
 
+	if (host->resetting)
+		return;
+
 	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_MSIRM(reg_id), BIT(reg_bit),
 			       0);
 }
@@ -972,6 +980,9 @@ static void rzg3s_pcie_intx_irq_mask(struct irq_data *d)
 
 	guard(raw_spinlock_irqsave)(&host->hw_lock);
 
+	if (host->resetting)
+		return;
+
 	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_PINTRCVIE,
 			       RZG3S_PCI_PINTRCVIE_INTX(d->hwirq), 0);
 }
@@ -982,6 +993,9 @@ static void rzg3s_pcie_intx_irq_unmask(struct irq_data *d)
 
 	guard(raw_spinlock_irqsave)(&host->hw_lock);
 
+	if (host->resetting)
+		return;
+
 	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_PINTRCVIE,
 			       RZG3S_PCI_PINTRCVIE_INTX(d->hwirq),
 			       RZG3S_PCI_PINTRCVIE_INTX(d->hwirq));
@@ -1185,12 +1199,46 @@ static void rzg3s_pcie_link_event(struct rzg3s_pcie_host *host, bool bounced)
 {
 	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(host);
 	struct pci_bus *bus = bridge->bus, *child;
+	unsigned long flags;
 	u32 val;
 	int ret;
 
-	pci_lock_rescan_remove();
+	/*
+	 * A latched DL_UpDown means at least one real link transition took
+	 * place since the last enumeration. If devices are enumerated
+	 * recover the Root Port first.
+	 */
+	if (bounced && !list_empty(&bus->devices)) {
+		struct pci_dev *port = pci_get_slot(bus, PCI_DEVFN(0, 0));
+
+		if (port && pci_pcie_type(port) == PCI_EXP_TYPE_ROOT_PORT)
+			pci_host_handle_link_down(port);
 
+		pci_dev_put(port);
+	}
+
+	raw_spin_lock_irqsave(&host->hw_lock, flags);
+
+	/*
+	 * Don't continue if reset in progress or .reset_root_port() failed to
+	 * start HW.
+	 */
+	if (host->resetting || !READ_ONCE(host->started)) {
+		raw_spin_unlock_irqrestore(&host->hw_lock, flags);
+		return;
+	}
+
+	/*
+	 * Check the link state after the recovery: the .reset_root_port()
+	 * retrains the link, so a device that is still present comes back
+	 * up here and is re-enumerated right away instead of waiting for
+	 * the next DL_UpDown event.
+	 */
 	val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT1);
+	raw_spin_unlock_irqrestore(&host->hw_lock, flags);
+
+	pci_lock_rescan_remove();
+
 	if (val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS) {
 		if (!list_empty(&bus->devices)) {
 			dev_info(host->dev,
@@ -1246,20 +1294,26 @@ static irqreturn_t rzg3s_pcie_event_irq_thread(int irq, void *data)
 {
 	struct rzg3s_pcie_host *host = data;
 	bool bounced;
-	u32 status;
 
-	status = readl_relaxed(host->axi + RZG3S_PCI_PEIS0);
-	bounced = status & RZG3S_PCI_PEIS0_DL_UPDOWN;
+	scoped_guard(raw_spinlock_irqsave, &host->hw_lock) {
+		u32 status;
 
-	if (!bounced && !READ_ONCE(host->link_rescan))
-		return IRQ_NONE;
+		if (host->resetting)
+			return IRQ_NONE;
+
+		status = readl_relaxed(host->axi + RZG3S_PCI_PEIS0);
+		bounced = status & RZG3S_PCI_PEIS0_DL_UPDOWN;
+
+		if (!bounced && !READ_ONCE(host->link_rescan))
+			return IRQ_NONE;
 
-	/* Clear the DL_UpDown status (W1C) */
-	if (bounced) {
-		writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN,
-			       host->axi + RZG3S_PCI_PEIS0);
+		/* Clear the DL_UpDown status (W1C) */
+		if (bounced) {
+			writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN,
+				       host->axi + RZG3S_PCI_PEIS0);
+		}
+		WRITE_ONCE(host->link_rescan, false);
 	}
-	WRITE_ONCE(host->link_rescan, false);
 
 	rzg3s_pcie_link_event(host, bounced);
 
@@ -1982,13 +2036,13 @@ static int rzg3s_pcie_host_stop(struct rzg3s_pcie_host *host)
 	return ret;
 }
 
-static int rzg3s_pcie_host_start(struct rzg3s_pcie_host *host)
+static int rzg3s_pcie_host_start(struct rzg3s_pcie_host *host, bool set_started)
 {
 	const struct rzg3s_pcie_soc_data *data = host->data;
 	struct rzg3s_sysc *sysc = host->sysc;
 	int ret;
 
-	if (host->started)
+	if (READ_ONCE(host->started))
 		return 0;
 
 	ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_MODE, 1);
@@ -2023,7 +2077,10 @@ static int rzg3s_pcie_host_start(struct rzg3s_pcie_host *host)
 	if (ret)
 		goto assert_power_resets;
 
-	host->started = true;
+	if (set_started) {
+		WRITE_ONCE(host->started, true);
+		host->resetting = false;
+	}
 
 	return 0;
 
@@ -2038,6 +2095,91 @@ static int rzg3s_pcie_host_start(struct rzg3s_pcie_host *host)
 	return ret;
 }
 
+static int rzg3s_pcie_host_reset_root_port(struct pci_host_bridge *bridge,
+					   struct pci_dev *pdev)
+{
+	struct rzg3s_pcie_host *host = pci_host_bridge_priv(bridge);
+	unsigned long flags;
+	u32 irqs, link_irqs;
+	int ret;
+
+	raw_spin_lock_irqsave(&host->hw_lock, flags);
+	if (host->resetting) {
+		raw_spin_unlock_irqrestore(&host->hw_lock, flags);
+		return -EBUSY;
+	}
+	host->resetting = true;
+	raw_spin_unlock_irqrestore(&host->hw_lock, flags);
+
+	/* Mask link up/down interrupts. */
+	link_irqs = readl_relaxed(host->axi + RZG3S_PCI_PEIE0);
+	writel_relaxed(0, host->axi + RZG3S_PCI_PEIE0);
+
+	/* Mask INTx and MSI interrupts. */
+	irqs = readl_relaxed(host->axi + RZG3S_PCI_PINTRCVIE);
+	writel_relaxed(0, host->axi + RZG3S_PCI_PINTRCVIE);
+
+	/* Flush IRQ masks. */
+	readl_relaxed(host->axi + RZG3S_PCI_PEIE0);
+	readl_relaxed(host->axi + RZG3S_PCI_PINTRCVIE);
+
+	/*
+	 * Make sure the next operations are not disturbed by any pending
+	 * IRQs.
+	 */
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		disable_irq(host->msi.irq);
+	for (unsigned int i = 0; i < PCI_NUM_INTX; i++)
+		disable_irq(host->intx_irqs[i]);
+
+	ret = rzg3s_pcie_host_stop(host);
+	if (ret) {
+		dev_err(host->dev, "Failed to stop the host!\n");
+		goto unmask_irqs;
+	}
+
+	ret = rzg3s_pcie_host_start(host, false);
+	if (ret) {
+		dev_err(host->dev, "Failed to start the host!\n");
+
+		/*
+		 * Don't unmask IRQs. We are in a bad state here and we
+		 * can recover only through a suspend/resume cycle. Just
+		 * re-enable the IRQs and return.
+		 */
+		goto enable_irqs;
+	}
+
+unmask_irqs:
+	/* Unmask INTx and MSI interrupts. */
+	writel_relaxed(irqs, host->axi + RZG3S_PCI_PINTRCVIE);
+
+	/*
+	 * rzg3s_pcie_host_start() retrains the link. If the link comes
+	 * back up, DL_UpDown gets latched in PEIS0. Clear the latch.
+	 * The link state is evaluated after this function returns, so
+	 * the event is not lost. For the same reason there is no need
+	 * to set host->link_rescan.
+	 */
+	writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIS0);
+	/* Unmask link up/down interrupts. */
+	writel_relaxed(link_irqs, host->axi + RZG3S_PCI_PEIE0);
+
+	raw_spin_lock_irqsave(&host->hw_lock, flags);
+	host->resetting = false;
+	raw_spin_unlock_irqrestore(&host->hw_lock, flags);
+
+	WRITE_ONCE(host->started, true);
+
+enable_irqs:
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		enable_irq(host->msi.irq);
+	for (unsigned int i = 0; i < PCI_NUM_INTX; i++)
+		enable_irq(host->intx_irqs[i]);
+
+	return ret;
+}
+
 static int rzg3s_pcie_get_controller_id(struct rzg3s_pcie_host *host)
 {
 	struct device_node *np = host->dev->of_node;
@@ -2113,18 +2255,22 @@ static int rzg3s_pcie_pm_notifier(struct notifier_block *nb,
 {
 	struct rzg3s_pcie_host *host = container_of(nb, struct rzg3s_pcie_host,
 						    pm_nb);
+	unsigned long flags;
 
 	switch (action) {
 	case PM_SUSPEND_PREPARE:
 		/* Disable link up/down interrupts. */
-		if (host->started)
+		raw_spin_lock_irqsave(&host->hw_lock, flags);
+		if (READ_ONCE(host->started) && !host->resetting)
 			writel_relaxed(0, host->axi + RZG3S_PCI_PEIE0);
+		raw_spin_unlock_irqrestore(&host->hw_lock, flags);
 		disable_irq(host->event_irq);
 		break;
 
 	case PM_POST_SUSPEND:
 		/* Enable link up/down interrupts and force link re-scan. */
-		if (host->started) {
+		raw_spin_lock_irqsave(&host->hw_lock, flags);
+		if (READ_ONCE(host->started) && !host->resetting) {
 			/*
 			 * The link re-train in resume triggers the link up IRQ
 			 * if there are connected devices. Clear the DL_UpDown
@@ -2137,8 +2283,9 @@ static int rzg3s_pcie_pm_notifier(struct notifier_block *nb,
 			writel_relaxed(RZG3S_PCI_PEIE0_DL_UPDOWN,
 				       host->axi + RZG3S_PCI_PEIE0);
 		}
+		raw_spin_unlock_irqrestore(&host->hw_lock, flags);
 		enable_irq(host->event_irq);
-		if (host->started)
+		if (READ_ONCE(host->started))
 			irq_wake_thread(host->event_irq, host);
 		break;
 	}
@@ -2253,6 +2400,7 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
 	bridge->sysdata = host;
 	bridge->ops = &rzg3s_pcie_root_ops;
 	bridge->child_ops = &rzg3s_pcie_child_ops;
+	bridge->reset_root_port = rzg3s_pcie_host_reset_root_port;
 	ret = pci_host_probe(bridge);
 	if (ret)
 		goto pm_notifier_unregister;
@@ -2311,7 +2459,7 @@ static int rzg3s_pcie_resume_noirq(struct device *dev)
 	struct rzg3s_pcie_host *host = dev_get_drvdata(dev);
 	int ret;
 
-	ret = rzg3s_pcie_host_start(host);
+	ret = rzg3s_pcie_host_start(host, true);
 	if (ret)
 		return ret;
 
-- 
2.43.0

Re: [PATCH v7 07/14] PCI: rzg3s-host: Use cached MSI address in .irq_compose_msi_msg()

[sashiko-bot]

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> The MSI window address is already cached in msi->window_base when the
> MSI window is configured. Use it to fill msg->address_lo and
> msg->address_hi in .irq_compose_msi_msg() instead of reading it back
> from the controller registers.
> 
> This avoids unnecessary register accesses.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019065025.2146539-1-claudiu.beznea@kernel.org?part=7

Re: [PATCH v7 06/14] PCI: rzg3s-host: Drop read-modify-write approach for the R/W1C registers

[sashiko-bot]

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> The AXI registers PCI_PINTRCVIS and PCI_PINTRCVIS_MSI are R/W1C. According
> to the RZ/G3S HW manual, Rev. 1.30, R/W1C means "it can be cleared to 0b by
> writing 1b. Writing 0b does not change anything."
> 
> Drop the read-modify-write approach and make use of this hardware
> capability. This avoids clearing unwanted interrupts.
> 
> Fixes: 7ef502fb35b2 ("PCI: Add Renesas RZ/G3S host controller driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019065025.2146539-1-claudiu.beznea@kernel.org?part=6

Re: [PATCH v7 03/14] PCI: rzg3s-host: Don't mix goto-based unwinding and cleanup helpers

[sashiko-bot]

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> rzg3s_pcie_probe() releases the sysc_np OF node through the
> __free(device_node) annotation, while the rest of the function uses
> goto-based unwinding. According to cleanup.h, the two styles should
> not be mixed within a function, as this makes the ownership semantics
> hard to follow. Drop the annotation and call of_node_put() explicitly
> on the exit paths instead.
> 
> Fixes: 7ef502fb35b2 ("PCI: Add Renesas RZ/G3S host controller driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019065025.2146539-1-claudiu.beznea@kernel.org?part=3

Re: [PATCH v7 08/14] PCI: rzg3s-host: Drop nop instructions

[sashiko-bot]

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> On the RZ/G3S PCIe IP variant (which is similar to those used on the
> RZ/G3E, RZ/V2H, and RZ/V2N SoCs), access to the PCIe Type 1 registers
> requires setting the PCI_PERM.CFG_HWINIT_EN bit. Since this bit is not
> set on the code paths where rzg3s_pcie_set_max_link_speed() is called,
> the writes to PCI_EXP_LNKCTL2.TLS are nops. Drop them.
> 
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019065025.2146539-1-claudiu.beznea@kernel.org?part=8

Re: [PATCH v7 12/14] PCI: rzg3s-host: Add helper to wait for link up

[sashiko-bot]

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Factor out the link up polling from rzg3s_pcie_host_init() into a new
> helper. With the link up/down event handling added by a subsequent
> commit, the probe will no longer fail when the link is down, but
> continue and rely on later link up events to detect the link becoming
> active. Having the polling in a dedicated helper allows the probe path
> to treat its failure as non-fatal.
> 
> No functional change.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019065025.2146539-1-claudiu.beznea@kernel.org?part=12

Re: [PATCH v7 09/14] PCI: rzg3s-host: Move host configuration code together

[sashiko-bot]

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Move host configuration code together to have it grouped. This prepares for
> the addition of hotplug support.
> 
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019065025.2146539-1-claudiu.beznea@kernel.org?part=9

Re: [PATCH v7 11/14] PCI: rzg3s-host: Move IRQ domain setup code

[sashiko-bot]

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Subsequent patches add support for the event IRQ to handle link
> up/down events. The event IRQ handler will use
> rzg3s_pcie_set_max_link_speed(). In preparation for adding event IRQ
> support, move the IRQ domain initialization code after
> rzg3s_pcie_set_max_link_speed().
> 
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019065025.2146539-1-claudiu.beznea@kernel.org?part=11

Re: [PATCH v7 04/14] PCI: rzg3s-host: Create the INTx IRQ domain before setting the chained handlers

[sashiko-bot]

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> rzg3s_pcie_init_irqdomain() installs the chained INTx handlers before
> creating the INTx IRQ domain. irq_set_chained_handler_and_data() also
> enables the interrupt, so an INTx interrupt arriving in between is
> handled by rzg3s_pcie_intx_irq_handler() while host->intx_domain is
> still NULL, leading to a NULL pointer dereference.
> 
> Move the IRQ domain creation before the loop that installs the chained
> handlers.
> 
> Fixes: 7ef502fb35b2 ("PCI: Add Renesas RZ/G3S host controller driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019065025.2146539-1-claudiu.beznea@kernel.org?part=4

Re: [PATCH v7 10/14] PCI: rzg3s-host: Move suspend/resume code into dedicated functions

[sashiko-bot]

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> In preparation for implementing hotplug using
> pci_host_bridge::reset_root_port(), move the suspend/resume code into
> rzg3s_pcie_host_stop() and rzg3s_pcie_host_start(). These functions
> will later be reused by the hotplug implementation through
> pci_host_bridge::reset_root_port().
> 
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019065025.2146539-1-claudiu.beznea@kernel.org?part=10

Re: [PATCH v7 02/14] PCI: rzg3s-host: Fix runtime PM handling in the NOIRQ suspend/resume phase

[sashiko-bot]

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> The runtime PM documentation states the following:
> - During system suspend, pm_runtime_get_noresume() is called for every
>   device right before executing the subsystem-level .prepare() callback
>   (in device_prepare()). In addition, the PM core disables runtime PM for
>   every device right before executing the subsystem-level .suspend_late()
>   callback (in device_suspend_late()).
> - During system resume, pm_runtime_enable() is called for every device
>   right after executing the subsystem-level .resume_early() callback (in
>   device_resume_early()), and pm_runtime_put() is called right after
>   executing the subsystem-level .complete() callback (in
>   device_complete()).
> 
> The driver's .suspend_noirq() callback is invoked after .suspend_late(),
> [ ... ]
> Fixes: 7ef502fb35b2 ("PCI: Add Renesas RZ/G3S host controller driver")
> Cc: stable@vger.kernel.org
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019065025.2146539-1-claudiu.beznea@kernel.org?part=2

Re: [PATCH v7 01/14] PCI: rzg3s-host: Follow hardware manual clock/reset initialization order

[sashiko-bot]

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> The RZ/G3S PCIe hardware manual specifies that the clocks must be enabled
> before the reset signals are deasserted during initialization.
> 
> Follow this sequence in the probe(), suspend(), and resume() paths to
> match the hardware requirements and avoid potential issues.
> 
> Fixes: 7ef502fb35b2 ("PCI: Add Renesas RZ/G3S host controller driver")
> Cc: stable@vger.kernel.org
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019065025.2146539-1-claudiu.beznea@kernel.org?part=1

Re: [PATCH v7 05/14] PCI: rzg3s-host: Initialize the IRQ domains after the IRQs are masked

[sashiko-bot]

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> The IRQ domain initialization requests the interrupts and installs the
> chained handlers before rzg3s_pcie_irq_init() masks the controller
> interrupt sources. An interrupt left enabled (e.g. by the bootloader,
> or latched from a previous controller state) could therefore fire
> before the controller initialization is complete and be handled on
> half-initialized state.
> 
> Move the IRQ domain initialization after rzg3s_pcie_irq_init(), so that
> all the interrupt sources are masked by the time the handlers are
> installed.
> 
> Fixes: 7ef502fb35b2 ("PCI: Add Renesas RZ/G3S host controller driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019065025.2146539-1-claudiu.beznea@kernel.org?part=5

Re: [PATCH v7 14/14] PCI: rzg3s-host: Add bridge::reset_root_port()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] PCI: rzg3s-host: unguarded MMIO access and lost INTx mask after start failure
- [High] PCI: rzg3s-host: lost MSI/INTx mask state across rzg3s_pcie_host_reset_root_port()
- [High] PCI: rzg3s-host: race and lost link event in rzg3s_pcie_host_reset_root_port()
- [Medium] PCI: rzg3s-host: race in rzg3s_pcie_host_reset_root_port() state ordering
- [Medium] PCI: rzg3s-host: race on RZG3S_PCI_PEIE0 between reset and PM notifier
- [Medium] PCI: rzg3s-host: ineffective disable_irq() on chained INTx IRQs in reset
--

commit 6658179b714958398bb0f41660bab61430893417
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

PCI: rzg3s-host: Add bridge::reset_root_port()

This commit implements pci_host_bridge::reset_root_port() for the Renesas
RZ/G3S PCIe host controller so the Root Port can be reset and recovered
across a link down/up cycle. It masks and disables MSI and INTx interrupts
around stopping and restarting the controller and adds a host->resetting
flag to block concurrent register accesses from the IRQ callbacks.

> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index a1c9e29f43e5d..f6823c9bc4149 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c

[ ... ]

> @@ -653,6 +655,9 @@ static void rzg3s_pcie_msi_irq_mask(struct irq_data *d)
>  
>  	guard(raw_spinlock_irqsave)(&host->hw_lock);
>  
> +	if (host->resetting)
> +		return;
> +
>  	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_MSIRM(reg_id), BIT(reg_bit),
>  			       BIT(reg_bit));
>  }
> @@ -666,6 +671,9 @@ static void rzg3s_pcie_msi_irq_unmask(struct irq_data *d)
>  
>  	guard(raw_spinlock_irqsave)(&host->hw_lock);
>  
> +	if (host->resetting)
> +		return;
> +
>  	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_MSIRM(reg_id), BIT(reg_bit),
>  			       0);
>  }
> @@ -972,6 +980,9 @@ static void rzg3s_pcie_intx_irq_mask(struct irq_data *d)
>  
>  	guard(raw_spinlock_irqsave)(&host->hw_lock);
>  
> +	if (host->resetting)
> +		return;
> +
>  	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_PINTRCVIE,
>  			       RZG3S_PCI_PINTRCVIE_INTX(d->hwirq), 0);
>  }
> @@ -982,6 +993,9 @@ static void rzg3s_pcie_intx_irq_unmask(struct irq_data *d)
>  
>  	guard(raw_spinlock_irqsave)(&host->hw_lock);
>  
> +	if (host->resetting)
> +		return;
> +
>  	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_PINTRCVIE,
>  			       RZG3S_PCI_PINTRCVIE_INTX(d->hwirq),
>  			       RZG3S_PCI_PINTRCVIE_INTX(d->hwirq));

[Severity: High]
Could returning early when host->resetting is true cause the hardware IRQ
masks in RZG3S_PCI_MSIRM(reg_id) and RZG3S_PCI_PINTRCVIE to go out of sync
with genirq?

In kernel/irq/chip.c, mask_irq() and unmask_irq() unconditionally update
IRQD_IRQ_MASKED after calling the chip callback. If an INTx or MSI vector is
masked or unmasked while host->resetting is true, these callbacks drop the
update without recording it, and rzg3s_pcie_host_reset_root_port() later
writes the pre-reset irqs snapshot back to RZG3S_PCI_PINTRCVIE.

In particular, rzg3s_pcie_host_reset_root_port() sets host->resetting = true
before reading RZG3S_PCI_PINTRCVIE into irqs (outside host->hw_lock) and
before calling disable_irq(host->intx_irqs[i]). If an INTx interrupt is in
flight on another CPU in handle_level_irq() or in a threaded IRQF_ONESHOT
handler on a child virq when irqs is snapshotted, its bit in
RZG3S_PCI_PINTRCVIE is 0. When that handler finishes and calls
cond_unmask_irq() or irq_finalize_oneshot() -> rzg3s_pcie_intx_irq_unmask(),
the unmask is ignored because host->resetting is true, and restoring the
stale irqs snapshot at unmask_irqs leaves the INTx line masked in hardware
while genirq considers it unmasked.

Also, because rzg3s_pcie_msi_parent_ops sets MSI_FLAG_PCI_MSI_MASK_PARENT,
per-vector MSI masking lives in RZG3S_PCI_MSIRM(reg_id) rather than
RZG3S_PCI_PINTRCVIE. Across rzg3s_pcie_host_stop() and
rzg3s_pcie_host_start(), RZG3S_PCI_MSIRM(reg_id) is reset and never saved or
restored.

> @@ -1185,12 +1199,46 @@ static void rzg3s_pcie_link_event(struct rzg3s_pcie_host *host, bool bounced)
>  {
>  	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(host);
>  	struct pci_bus *bus = bridge->bus, *child;
> +	unsigned long flags;
>  	u32 val;
>  	int ret;
>  
> -	pci_lock_rescan_remove();
> +	/*
> +	 * A latched DL_UpDown means at least one real link transition took
> +	 * place since the last enumeration. If devices are enumerated
> +	 * recover the Root Port first.
> +	 */
> +	if (bounced && !list_empty(&bus->devices)) {
> +		struct pci_dev *port = pci_get_slot(bus, PCI_DEVFN(0, 0));
> +
> +		if (port && pci_pcie_type(port) == PCI_EXP_TYPE_ROOT_PORT)
> +			pci_host_handle_link_down(port);
>  
> +		pci_dev_put(port);
> +	}
> +
> +	raw_spin_lock_irqsave(&host->hw_lock, flags);
> +
> +	/*
> +	 * Don't continue if reset in progress or .reset_root_port() failed to
> +	 * start HW.
> +	 */
> +	if (host->resetting || !READ_ONCE(host->started)) {
> +		raw_spin_unlock_irqrestore(&host->hw_lock, flags);
> +		return;
> +	}
> +
> +	/*
> +	 * Check the link state after the recovery: the .reset_root_port()
> +	 * retrains the link, so a device that is still present comes back
> +	 * up here and is re-enumerated right away instead of waiting for
> +	 * the next DL_UpDown event.
> +	 */
>  	val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT1);
> +	raw_spin_unlock_irqrestore(&host->hw_lock, flags);

[Severity: High]
What prevents a concurrent rzg3s_pcie_host_reset_root_port() from stopping
the controller after host->hw_lock is released here while
rzg3s_pcie_link_event() is still accessing controller registers?

bridge->reset_root_port can be invoked outside rzg3s_pcie_link_event()
through pci_bridge_secondary_bus_reset() -> pcibios_reset_secondary_bus()
(for example via sysfs reset_subordinate, pci_reset_function(),
pci_reset_bus(), or aer_root_reset()). Once rzg3s_pcie_link_event() drops
host->hw_lock after reading RZG3S_PCI_PCSTAT1, it proceeds to touch
host->axi and host->pcie in rzg3s_pcie_device_replaced(),
rzg3s_pcie_set_max_link_speed(), pci_scan_child_bus(), and
rzg3s_pcie_cache_connected_dev_id(). Because rzg3s_pcie_host_reset_root_port()
only takes host->hw_lock briefly to set host->resetting = true and does not
synchronize with host->event_irq, it can enter rzg3s_pcie_host_stop() and
assert power_resets while rzg3s_pcie_link_event() is in the middle of those
MMIO accesses.

Conversely, if rzg3s_pcie_event_irq_thread() clears RZG3S_PCI_PEIS0_DL_UPDOWN
and host->link_rescan and then rzg3s_pcie_link_event() returns early here
because a concurrent reset set host->resetting = true, won't that link event
or rescan request be lost?

[ ... ]

> @@ -2038,6 +2095,91 @@ static int rzg3s_pcie_host_start(struct rzg3s_pcie_host *host)
>  	return ret;
>  }
>  
> +static int rzg3s_pcie_host_reset_root_port(struct pci_host_bridge *bridge,
> +					   struct pci_dev *pdev)
> +{
> +	struct rzg3s_pcie_host *host = pci_host_bridge_priv(bridge);
> +	unsigned long flags;
> +	u32 irqs, link_irqs;
> +	int ret;
> +
> +	raw_spin_lock_irqsave(&host->hw_lock, flags);
> +	if (host->resetting) {
> +		raw_spin_unlock_irqrestore(&host->hw_lock, flags);
> +		return -EBUSY;
> +	}
> +	host->resetting = true;
> +	raw_spin_unlock_irqrestore(&host->hw_lock, flags);
> +
> +	/* Mask link up/down interrupts. */
> +	link_irqs = readl_relaxed(host->axi + RZG3S_PCI_PEIE0);
> +	writel_relaxed(0, host->axi + RZG3S_PCI_PEIE0);
> +
> +	/* Mask INTx and MSI interrupts. */
> +	irqs = readl_relaxed(host->axi + RZG3S_PCI_PINTRCVIE);
> +	writel_relaxed(0, host->axi + RZG3S_PCI_PINTRCVIE);
> +
> +	/* Flush IRQ masks. */
> +	readl_relaxed(host->axi + RZG3S_PCI_PEIE0);
> +	readl_relaxed(host->axi + RZG3S_PCI_PINTRCVIE);
> +
> +	/*
> +	 * Make sure the next operations are not disturbed by any pending
> +	 * IRQs.
> +	 */
> +	if (IS_ENABLED(CONFIG_PCI_MSI))
> +		disable_irq(host->msi.irq);
> +	for (unsigned int i = 0; i < PCI_NUM_INTX; i++)
> +		disable_irq(host->intx_irqs[i]);

[Severity: Medium]
Does disable_irq(host->intx_irqs[i]) actually mask or synchronize the INTx
interrupts here?

In rzg3s_pcie_init_irqdomain(), host->intx_irqs[i] are registered with
irq_set_chained_handler_and_data(irq, rzg3s_pcie_intx_irq_handler, host)
without IRQ_DISABLE_UNLAZY. On GICv3, irq_chip::irq_disable is NULL, so
__irq_disable() only sets IRQD_IRQ_DISABLED (lazy disable) without masking
the GIC SPI. And because chained handlers replace desc->handle_irq with
rzg3s_pcie_intx_irq_handler(), generic_handle_irq_desc() invokes
rzg3s_pcie_intx_irq_handler() directly without checking irqd_irq_disabled()
or setting IRQD_IRQ_INPROGRESS.

If an INTx SPI is already pending at the GIC before RZG3S_PCI_PINTRCVIE is
cleared, or glitches while resets are toggled, rzg3s_pcie_intx_irq_handler()
can still run during reset and call generic_handle_domain_irq() ->
handle_level_irq() -> mask_ack_irq() -> rzg3s_pcie_intx_irq_ack(), which
writes to host->axi + RZG3S_PCI_PINTRCVIS without checking host->resetting.

> +
> +	ret = rzg3s_pcie_host_stop(host);
> +	if (ret) {
> +		dev_err(host->dev, "Failed to stop the host!\n");
> +		goto unmask_irqs;
> +	}
> +
> +	ret = rzg3s_pcie_host_start(host, false);
> +	if (ret) {
> +		dev_err(host->dev, "Failed to start the host!\n");
> +
> +		/*
> +		 * Don't unmask IRQs. We are in a bad state here and we
> +		 * can recover only through a suspend/resume cycle. Just
> +		 * re-enable the IRQs and return.
> +		 */
> +		goto enable_irqs;
> +	}

[Severity: High]
If rzg3s_pcie_host_start(host, false) fails here and jumps to enable_irqs,
won't subsequent callers access host->pcie and host->axi while power_resets
are asserted and RST_RSM_B is 0?

When rzg3s_pcie_host_reset_root_port() returns an error to
pcibios_reset_secondary_bus(), pci_bridge_secondary_bus_reset() still calls
pci_bridge_wait_for_secondary_bus(), which immediately polls config space
through rzg3s_pcie_root_ops (rzg3s_pcie_root_map_bus()) and
rzg3s_pcie_child_ops (rzg3s_pcie_child_read()), neither of which checks
host->started or host->resetting. Moreover, rzg3s_pcie_link_event() returns
early on !READ_ONCE(host->started) without removing or marking child devices
disconnected, so endpoint drivers and pci_pm_suspend() during the recovery
suspend/resume cycle will also access config space and MMIO while the IP is
held in reset.

In addition, jumping to enable_irqs discards the local irqs variable holding
the saved RZG3S_PCI_PINTRCVIE value. When rzg3s_pcie_resume_noirq() later
calls rzg3s_pcie_host_start(host, true), only RZG3S_PCI_PINTRCVIE_MSI is set
and the INTx enable bits in RZG3S_PCI_PINTRCVIE are never restored. And if
rzg3s_pcie_resume_noirq() itself fails in rzg3s_pcie_host_start(host, true),
host->started remains false while host->resetting is false, so
rzg3s_pcie_host_reset_root_port(), rzg3s_pcie_msi_irq_mask/unmask(),
rzg3s_pcie_intx_irq_mask/unmask(), and rzg3s_pcie_event_irq_thread() (which
only check host->resetting) will access host->axi while the IP is not
started.

> +
> +unmask_irqs:
> +	/* Unmask INTx and MSI interrupts. */
> +	writel_relaxed(irqs, host->axi + RZG3S_PCI_PINTRCVIE);
> +
> +	/*
> +	 * rzg3s_pcie_host_start() retrains the link. If the link comes
> +	 * back up, DL_UpDown gets latched in PEIS0. Clear the latch.
> +	 * The link state is evaluated after this function returns, so
> +	 * the event is not lost. For the same reason there is no need
> +	 * to set host->link_rescan.
> +	 */
> +	writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIS0);

[Severity: High]
Is it safe to clear RZG3S_PCI_PEIS0_DL_UPDOWN here without setting
host->link_rescan or waking host->event_irq when .reset_root_port() is
called outside rzg3s_pcie_link_event()?

When rzg3s_pcie_host_reset_root_port() is invoked via
pci_bridge_secondary_bus_reset() -> pcibios_reset_secondary_bus() from a path
other than rzg3s_pcie_link_event() (such as pci_reset_function(),
pci_reset_bus(), or sysfs reset_subordinate), nothing evaluates the link
state after this function returns, so clearing RZG3S_PCI_PEIS0_DL_UPDOWN
drops any link transition that occurred during the reset.

> +	/* Unmask link up/down interrupts. */
> +	writel_relaxed(link_irqs, host->axi + RZG3S_PCI_PEIE0);
> +
> +	raw_spin_lock_irqsave(&host->hw_lock, flags);
> +	host->resetting = false;
> +	raw_spin_unlock_irqrestore(&host->hw_lock, flags);
> +
> +	WRITE_ONCE(host->started, true);

[Severity: Medium]
Can the ordering between unmasking RZG3S_PCI_PEIE0, clearing
host->resetting, and setting host->started = true cause false failure
detection or a dropped link interrupt?

First, RZG3S_PCI_PEIE0 is unmasked before host->resetting is cleared, while
host->event_irq remains enabled throughout rzg3s_pcie_host_reset_root_port().
If a link event fires right after RZG3S_PCI_PEIE0 is written,
rzg3s_pcie_event_irq_thread() can run while host->resetting is still true
and return IRQ_NONE.

Second, host->resetting = false is committed and host->hw_lock is released
before WRITE_ONCE(host->started, true) (since rzg3s_pcie_host_stop() set
host->started = false and rzg3s_pcie_host_start(host, false) was called with
set_started = false). In that window, both host->resetting and host->started
are false, so a reader holding host->hw_lock in rzg3s_pcie_link_event():

	if (host->resetting || !READ_ONCE(host->started)) {
		raw_spin_unlock_irqrestore(&host->hw_lock, flags);
		return;
	}

or in rzg3s_pcie_pm_notifier() can observe host->resetting == false and
host->started == false and treat a successful reset as a failed hardware
start. Note that rzg3s_pcie_host_start() (when set_started is true) and
rzg3s_pcie_host_stop() also update host->started and host->resetting without
holding host->hw_lock.

> +
> +enable_irqs:
> +	if (IS_ENABLED(CONFIG_PCI_MSI))
> +		enable_irq(host->msi.irq);
> +	for (unsigned int i = 0; i < PCI_NUM_INTX; i++)
> +		enable_irq(host->intx_irqs[i]);
> +
> +	return ret;
> +}
> +
>  static int rzg3s_pcie_get_controller_id(struct rzg3s_pcie_host *host)
>  {
>  	struct device_node *np = host->dev->of_node;
> @@ -2113,18 +2255,22 @@ static int rzg3s_pcie_pm_notifier(struct notifier_block *nb,
>  {
>  	struct rzg3s_pcie_host *host = container_of(nb, struct rzg3s_pcie_host,
>  						    pm_nb);
> +	unsigned long flags;
>  
>  	switch (action) {
>  	case PM_SUSPEND_PREPARE:
>  		/* Disable link up/down interrupts. */
> -		if (host->started)
> +		raw_spin_lock_irqsave(&host->hw_lock, flags);
> +		if (READ_ONCE(host->started) && !host->resetting)
>  			writel_relaxed(0, host->axi + RZG3S_PCI_PEIE0);
> +		raw_spin_unlock_irqrestore(&host->hw_lock, flags);
>  		disable_irq(host->event_irq);
>  		break;
>  
>  	case PM_POST_SUSPEND:
>  		/* Enable link up/down interrupts and force link re-scan. */
> -		if (host->started) {
> +		raw_spin_lock_irqsave(&host->hw_lock, flags);
> +		if (READ_ONCE(host->started) && !host->resetting) {
>  			/*
>  			 * The link re-train in resume triggers the link up IRQ
>  			 * if there are connected devices. Clear the DL_UpDown
> @@ -2137,8 +2283,9 @@ static int rzg3s_pcie_pm_notifier(struct notifier_block *nb,
>  			writel_relaxed(RZG3S_PCI_PEIE0_DL_UPDOWN,
>  				       host->axi + RZG3S_PCI_PEIE0);
>  		}
> +		raw_spin_unlock_irqrestore(&host->hw_lock, flags);
>  		enable_irq(host->event_irq);
> -		if (host->started)
> +		if (READ_ONCE(host->started))
>  			irq_wake_thread(host->event_irq, host);
>  		break;
>  	}

[Severity: Medium]
Can rzg3s_pcie_pm_notifier() and rzg3s_pcie_host_reset_root_port() race on
RZG3S_PCI_PEIE0?

PM_SUSPEND_PREPARE runs before tasks are frozen or devices are suspended. If
rzg3s_pcie_host_reset_root_port() is already running, it saves link_irqs =
RZG3S_PCI_PEIE0_DL_UPDOWN and sets host->resetting = true. When
rzg3s_pcie_pm_notifier(PM_SUSPEND_PREPARE) runs concurrently, it sees
host->resetting == true and skips writing 0 to RZG3S_PCI_PEIE0, and
rzg3s_pcie_host_reset_root_port() then writes link_irqs back to
RZG3S_PCI_PEIE0 at unmask_irqs, leaving link interrupts unmasked in hardware
during suspend.

Conversely, if PM_SUSPEND_PREPARE runs first and writes 0 to
RZG3S_PCI_PEIE0, and a reset is then triggered before devices suspend,
rzg3s_pcie_host_reset_root_port() saves link_irqs = 0. If suspend aborts and
PM_POST_SUSPEND runs while the reset is still in progress (host->resetting
== true, host->started == false), PM_POST_SUSPEND skips re-enabling
RZG3S_PCI_PEIE0_DL_UPDOWN and skips irq_wake_thread(), and
rzg3s_pcie_host_reset_root_port() restores link_irqs = 0 at unmask_irqs,
leaving link up/down interrupts disabled after resume.

[ ... ]

>
> -- 
> 2.43.0
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019065025.2146539-1-claudiu.beznea@kernel.org?part=14

Re: [PATCH v7 13/14] PCI: rzg3s-host: Re-enumerate the bus on PCIe link-state changes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] pci: rzg3s-host: rzg3s_pcie_remove_devices() marks Root Port disconnected
- [High] pci: rzg3s-host: rzg3s_pcie_pm_notifier() misses hibernation PM events
- [High] PCI: rzg3s-host: race between rzg3s_pcie_probe() and PM notifier callbacks
- [High] PCI: rzg3s-host: deadlock on pm_chain_head.rwsem in rzg3s_pcie_pm_notifier()
- [Medium] pci: rzg3s-host: late PEIS0_DL_UPDOWN clear loses link bounce events
- [Medium] pci: rzg3s-host: rzg3s_pcie_link_event() reads config before link settles
- [Medium] PCI: rzg3s-host: controller registers accessed after resume_noirq() failure
--

commit 1730fab066afd00592ed2f6ea77d04307ed48cf0
Author: John Madieu <john.madieu.xa@bp.renesas.com>

PCI: rzg3s-host: Re-enumerate the bus on PCIe link-state changes

This commit enables the DL_UpDown interrupt on RZ/G3S and related PCIe
controllers to detect link-state transitions and hotplug-like events,
removing or rescanning devices on the bus accordingly. It also adds a PM
notifier to disable link event interrupts across system suspend and force a
bus rescan on resume, and makes probe tolerant of an initially down link.

> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index 1ced6d83c7c18..a1c9e29f43e5d 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c

[ ... ]

> @@ -1094,12 +1108,187 @@ static int rzg3s_pcie_wait_for_link_up(struct rzg3s_pcie_host *host)

[ ... ]

> +static void rzg3s_pcie_remove_devices(struct pci_bus *bus)
> +{
> +	struct pci_dev *dev, *tmp;
> +
> +	list_for_each_entry_safe_reverse(dev, tmp, &bus->devices, bus_list) {
> +		pci_dev_set_disconnected(dev, NULL);
> +		if (pci_has_subordinate(dev)) {
> +			pci_walk_bus(dev->subordinate, pci_dev_set_disconnected,
> +				     NULL);
> +		}
> +		pci_stop_and_remove_bus_device(dev);
> +	}
> +}

[Severity: High]
Both callers in rzg3s_pcie_link_event() pass bridge->bus (the root bus) to
rzg3s_pcie_remove_devices(), where dev in &bus->devices is the controller's
own integrated Root Port (0000:00:00.0) rather than an endpoint across the
link.

Should pci_dev_set_disconnected() only be called on dev->subordinate (as
done in rzg3s_pcie_resume_noirq()) rather than on dev itself?

Because pci_read_config_*() and pci_write_config_*() immediately abort with
PCIBIOS_DEVICE_NOT_FOUND when pci_dev_is_disconnected(dev) is true, marking
the Root Port as disconnected before pci_stop_and_remove_bus_device(dev)
causes Root Port teardown (such as pcie_aspm_exit_link_state(),
pcie_pme_remove(), aer_remove(), pci_msi_shutdown(), and
do_pci_disable_device()) to skip clearing hardware state in the Root Port's
local MMIO registers (host->pcie), leaving stale ASPM, PME, AER, MSI, and
PCI_COMMAND settings across subsequent re-enumerations.

> +
> +static void rzg3s_pcie_link_event(struct rzg3s_pcie_host *host, bool bounced)
> +{
> +	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(host);
> +	struct pci_bus *bus = bridge->bus, *child;
> +	u32 val;
> +	int ret;
> +
> +	pci_lock_rescan_remove();
> +
> +	val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT1);
> +	if (val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS) {
> +		if (!list_empty(&bus->devices)) {
> +			dev_info(host->dev,
> +				 "PCIe link down, removing devices\n");
> +			rzg3s_pcie_remove_devices(bus);
> +		}
> +		goto unlock;
> +	}
> +
> +	if (!list_empty(&bus->devices)) {
> +		bool replaced = rzg3s_pcie_device_replaced(host);
> +
> +		/*
> +		 * If bounced = false it means link rescanning (after probe
> +		 * or system resume), no link transition was recorded. The
> +		 * enumerated devices are intact, so remove them only if the
> +		 * connected device was replaced while link events were not
> +		 * delivered.
> +		 *
> +		 * If bounced = true it means at least one real link down/up
> +		 * cycle occurred.
> +		 */
> +		if (!bounced && !replaced)
> +			goto unlock;
> +
> +		rzg3s_pcie_remove_devices(bus);
> +	}
> +
> +	dev_info(host->dev, "PCIe link up, rescanning bus\n");
> +
> +	ret = rzg3s_pcie_set_max_link_speed(host);
> +	if (ret)
> +		dev_info(host->dev, "Failed to set max link speed\n");
> +
> +	pci_host_common_link_train_delay(host->max_link_speed);

[Severity: Medium]
When bounced is true and !list_empty(&bus->devices), does calling
rzg3s_pcie_device_replaced(host) issue Type 0 configuration reads to the
downstream endpoint before the link has finished settling?

When a real DL_UpDown event occurs (bounced == true), the result of
rzg3s_pcie_device_replaced(host) is not needed because the devices are
removed unconditionally, and rzg3s_pcie_set_max_link_speed() (which polls
for LTSSM L0 and negotiates link speed) and
pci_host_common_link_train_delay() have not been called yet. Should
rzg3s_pcie_device_replaced(host) only be called when !bounced?

[ ... ]

> @@ -1901,6 +2108,44 @@ static void rzv2h_pcie_release_lanes(void *data)
>  	rzv2h_num_total_lanes -= host->num_lanes;
>  }
>  
> +static int rzg3s_pcie_pm_notifier(struct notifier_block *nb,
> +				  unsigned long action, void *data)
> +{
> +	struct rzg3s_pcie_host *host = container_of(nb, struct rzg3s_pcie_host,
> +						    pm_nb);
> +
> +	switch (action) {
> +	case PM_SUSPEND_PREPARE:
> +		/* Disable link up/down interrupts. */
> +		if (host->started)
> +			writel_relaxed(0, host->axi + RZG3S_PCI_PEIE0);
> +		disable_irq(host->event_irq);
> +		break;

[Severity: High]
Can calling disable_irq(host->event_irq) inside rzg3s_pcie_pm_notifier()
deadlock on pm_chain_head.rwsem if rzg3s_pcie_event_irq_thread() is
concurrently probing or removing a PCI device whose driver registers or
unregisters a PM notifier?

During system suspend:

suspend_prepare()
  pm_notifier_call_chain_robust(PM_SUSPEND_PREPARE, PM_POST_SUSPEND)
    blocking_notifier_call_chain_robust(&pm_chain_head, ...)
      down_read(&pm_chain_head.rwsem)
      rzg3s_pcie_pm_notifier()
        disable_irq(host->event_irq)
          synchronize_irq(host->event_irq)

If rzg3s_pcie_event_irq_thread() is simultaneously running
rzg3s_pcie_link_event(), it calls pci_bus_add_devices() or
rzg3s_pcie_remove_devices() -> pci_stop_and_remove_bus_device(), which
synchronously runs PCI endpoint driver .probe() or .remove() callbacks.
Several PCI drivers (such as ath11k_pci, wil6210, amdgpu, and xe) call
register_pm_notifier() or unregister_pm_notifier() in those callbacks,
which blocks on down_write(&pm_chain_head.rwsem) while the suspend thread
holds down_read(&pm_chain_head.rwsem) waiting for the IRQ thread to finish.

> +
> +	case PM_POST_SUSPEND:
> +		/* Enable link up/down interrupts and force link re-scan. */
> +		if (host->started) {
> +			/*
> +			 * The link re-train in resume triggers the link up IRQ
> +			 * if there are connected devices. Clear the DL_UpDown
> +			 * status and trigger link_rescan to avoid
> +			 * re-enumerating the already connected devices.
> +			 */
> +			writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN,
> +				       host->axi + RZG3S_PCI_PEIS0);
> +			WRITE_ONCE(host->link_rescan, true);
> +			writel_relaxed(RZG3S_PCI_PEIE0_DL_UPDOWN,
> +				       host->axi + RZG3S_PCI_PEIE0);
> +		}
> +		enable_irq(host->event_irq);
> +		if (host->started)
> +			irq_wake_thread(host->event_irq, host);
> +		break;
> +	}
> +
> +	return NOTIFY_DONE;
> +}

[Severity: High]
rzg3s_pcie_pm_ops uses NOIRQ_SYSTEM_SLEEP_PM_OPS(rzg3s_pcie_suspend_noirq,
rzg3s_pcie_resume_noirq), which wires up the freeze_noirq, thaw_noirq,
poweroff_noirq, and restore_noirq callbacks for hibernation. However,
rzg3s_pcie_pm_notifier() only handles PM_SUSPEND_PREPARE and
PM_POST_SUSPEND.

During hibernation, won't omitting PM_HIBERNATION_PREPARE /
PM_RESTORE_PREPARE leave host->event_irq enabled during device freeze and
poweroff, and won't omitting PM_POST_HIBERNATION / PM_POST_RESTORE leave
RZG3S_PCI_PEIE0 cleared to 0 (after rzg3s_pcie_resume_noirq() ->
rzg3s_pcie_host_start() -> rzg3s_pcie_irq_init() resets it) and skip waking
the IRQ thread to rescan the bus?

>  
>  static int rzg3s_pcie_probe(struct platform_device *pdev)
>  {

[ ... ]

> @@ -1999,15 +2244,38 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto power_resets_assert;
>  
> +	host->started = true;
> +	host->pm_nb.notifier_call = rzg3s_pcie_pm_notifier;
> +	ret = register_pm_notifier(&host->pm_nb);
> +	if (ret)
> +		goto host_probe_teardown;
> +
>  	bridge->sysdata = host;
>  	bridge->ops = &rzg3s_pcie_root_ops;
>  	bridge->child_ops = &rzg3s_pcie_child_ops;
>  	ret = pci_host_probe(bridge);
>  	if (ret)
> -		goto host_probe_teardown;
> +		goto pm_notifier_unregister;
> +
> +	pci_lock_rescan_remove();
> +	rzg3s_pcie_cache_connected_dev_id(host);
> +	pci_unlock_rescan_remove();
> +
> +	/*
> +	 * The link re-train triggers the link up IRQ if there are connected
> +	 * devices. Clear DL_UpDown status and trigger link_rescan to avoid
> +	 * re-enumerating the devices detected while pci_host_probe() and loose
> +	 * any potential event.
> +	 */
> +	writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIS0);
> +	WRITE_ONCE(host->link_rescan, true);
> +	writel_relaxed(RZG3S_PCI_PEIE0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIE0);
> +	irq_wake_thread(host->event_irq, host);
>  
>  	return 0;

[Severity: High]
Because rzg3s_pcie_driver sets PROBE_PREFER_ASYNCHRONOUS, and system
suspend runs pm_notifier_call_chain_robust(PM_SUSPEND_PREPARE,
PM_POST_SUSPEND) before dpm_prepare() calls wait_for_device_probe(), can
rzg3s_pcie_probe() race with rzg3s_pcie_pm_notifier()?

1. host->started is set to true and register_pm_notifier(&host->pm_nb) is
called before pci_host_probe(bridge) initializes bridge->bus. If
suspend_prepare() aborts before pci_register_host_bridge() sets
bridge->bus, PM_POST_SUSPEND wakes rzg3s_pcie_event_irq_thread() ->
rzg3s_pcie_link_event(), which dereferences bridge->bus while it is still
NULL.

2. If PM_SUSPEND_PREPARE runs while rzg3s_pcie_probe() is inside
pci_host_probe(), rzg3s_pcie_pm_notifier() clears RZG3S_PCI_PEIE0 and calls
disable_irq(host->event_irq), but rzg3s_pcie_probe() then finishes before
wait_for_device_probe() returns and unconditionally enables
RZG3S_PCI_PEIE0_DL_UPDOWN and calls irq_wake_thread(host->event_irq, host),
waking the IRQ thread during suspend.

3. If PM_SUSPEND_PREPARE runs before register_pm_notifier(&host->pm_nb) is
called in rzg3s_pcie_probe(), PM_POST_SUSPEND will later call
enable_irq(host->event_irq) without a matching disable_irq().

[Severity: Medium]
In addition, clearing RZG3S_PCI_PEIS0_DL_UPDOWN here at the end of
rzg3s_pcie_probe() (after pci_host_probe(bridge)) and in
rzg3s_pcie_pm_notifier() under PM_POST_SUSPEND (after all device resume
callbacks or after an aborted suspend), rather than immediately after link
training in rzg3s_pcie_host_setup(), seems able to lose link bounce events:

If the same endpoint experiences a link down/up bounce during
pci_host_probe(), between rzg3s_pcie_resume_noirq() and PM_POST_SUSPEND, or
during an aborted suspend, clearing RZG3S_PCI_PEIS0_DL_UPDOWN discards the
hardware event so bounced is false. Because the same device is still
attached, rzg3s_pcie_device_replaced() returns false and
rzg3s_pcie_link_event() exits at:

	if (!bounced && !replaced)
		goto unlock;

leaving the endpoint with unconfigured BARs, PCI_COMMAND, and MSI state (or
at Gen1 speed if link-up raced with rzg3s_pcie_wait_for_link_up() timing
out in rzg3s_pcie_host_setup()). Should RZG3S_PCI_PEIS0_DL_UPDOWN be
cleared in rzg3s_pcie_host_setup() right after link training instead?

[ ... ]

> @@ -2041,8 +2309,25 @@ static int rzg3s_pcie_suspend_noirq(struct device *dev)
>  static int rzg3s_pcie_resume_noirq(struct device *dev)
>  {
>  	struct rzg3s_pcie_host *host = dev_get_drvdata(dev);
> +	int ret;
>  
> -	return rzg3s_pcie_host_start(host);
> +	ret = rzg3s_pcie_host_start(host);
> +	if (ret)
> +		return ret;
> +
> +	if (rzg3s_pcie_device_replaced(host)) {
> +		struct pci_host_bridge *bridge =
> +			pci_host_bridge_from_priv(host);
> +		struct pci_dev *port __free(pci_dev_put) =
> +			pci_get_slot(bridge->bus, PCI_DEVFN(0, 0));
> +
> +		if (port && port->subordinate) {
> +			pci_walk_bus(port->subordinate,
> +				     pci_dev_set_disconnected, NULL);
> +		}
> +	}
> +
> +	return 0;
>  }

[Severity: Medium]
If rzg3s_pcie_host_start(host) fails in rzg3s_pcie_resume_noirq(), its
error path asserts host->power_resets and clears
RZG3S_SYSC_FUNC_ID_RST_RSM_B, leaving host->started as false.

Although rzg3s_pcie_pm_notifier() checks host->started to avoid touching
controller registers after a failed resume, the PM core still runs
pci_pm_resume_noirq() and pci_pm_resume() on the child PCI devices. Since
rzg3s_pcie_resume_noirq() returns early before marking the devices
disconnected, and neither rzg3s_pcie_root_ops nor rzg3s_pcie_child_ops
checks host->started, won't config-space accesses during child device
resume (and later runtime accesses to the remaining PCI devices) still read
and write host->pcie and host->axi while the controller is held in reset?

Should the devices also be marked disconnected (or config accesses guarded
by host->started) when rzg3s_pcie_host_start() fails?

[ ... ]

完美！现在检查所有规则：
1. Plain text only, no markdown headers, no backticks, no ALL CAPS shouting? Checked.
2. Commit header format: `commit 1730fab066afd00592ed2f6ea77d04307ed48cf0`, `Author: John Madieu <john.madieu.xa@bp.renesas.com>`, one line subject, brief summary (2 sentences), unified diff quoted with `> `. Checked.
3. No line numbers mentioned in comments? Checked!
4. Wrapped at 78 characters for added text? Checked.
5. `[Severity: <level>]` on its own line right above each inline comment block? Checked (VF1: High, VF4: Medium, PVF3: High, VF2: High, PVF1: High, VF3: Medium, PVF2: Medium - all 7 findings included).
6. Ends with a blank line? Yes, I will make sure the output ends with a blank line and contains only the report. Let's output the final text now.commit 1730fab066afd00592ed2f6ea77d04307ed48cf0
Author: John Madieu <john.madieu.xa@bp.renesas.com>

PCI: rzg3s-host: Re-enumerate the bus on PCIe link-state changes

This commit enables the DL_UpDown interrupt on RZ/G3S and related PCIe
controllers to detect link-state transitions and hotplug-like events,
removing or rescanning devices on the bus accordingly. It also adds a PM
notifier to disable link event interrupts across system suspend and force a
bus rescan on resume, and makes probe tolerant of an initially down link.

> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index 1ced6d83c7c18..a1c9e29f43e5d 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c

[ ... ]

> @@ -1094,12 +1108,187 @@ static int rzg3s_pcie_wait_for_link_up(struct rzg3s_pcie_host *host)

[ ... ]

> +static void rzg3s_pcie_remove_devices(struct pci_bus *bus)
> +{
> +	struct pci_dev *dev, *tmp;
> +
> +	list_for_each_entry_safe_reverse(dev, tmp, &bus->devices, bus_list) {
> +		pci_dev_set_disconnected(dev, NULL);
> +		if (pci_has_subordinate(dev)) {
> +			pci_walk_bus(dev->subordinate, pci_dev_set_disconnected,
> +				     NULL);
> +		}
> +		pci_stop_and_remove_bus_device(dev);
> +	}
> +}

[Severity: High]
Both callers in rzg3s_pcie_link_event() pass bridge->bus (the root bus) to
rzg3s_pcie_remove_devices(), where dev in &bus->devices is the controller's
own integrated Root Port (0000:00:00.0) rather than an endpoint across the
link.

Should pci_dev_set_disconnected() only be called on dev->subordinate (as
done in rzg3s_pcie_resume_noirq()) rather than on dev itself?

Because pci_read_config_*() and pci_write_config_*() immediately abort with
PCIBIOS_DEVICE_NOT_FOUND when pci_dev_is_disconnected(dev) is true, marking
the Root Port as disconnected before pci_stop_and_remove_bus_device(dev)
causes Root Port teardown (such as pcie_aspm_exit_link_state(),
pcie_pme_remove(), aer_remove(), pci_msi_shutdown(), and
do_pci_disable_device()) to skip clearing hardware state in the Root Port's
local MMIO registers (host->pcie), leaving stale ASPM, PME, AER, MSI, and
PCI_COMMAND settings across subsequent re-enumerations.

> +
> +static void rzg3s_pcie_link_event(struct rzg3s_pcie_host *host, bool bounced)
> +{
> +	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(host);
> +	struct pci_bus *bus = bridge->bus, *child;
> +	u32 val;
> +	int ret;
> +
> +	pci_lock_rescan_remove();
> +
> +	val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT1);
> +	if (val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS) {
> +		if (!list_empty(&bus->devices)) {
> +			dev_info(host->dev,
> +				 "PCIe link down, removing devices\n");
> +			rzg3s_pcie_remove_devices(bus);
> +		}
> +		goto unlock;
> +	}
> +
> +	if (!list_empty(&bus->devices)) {
> +		bool replaced = rzg3s_pcie_device_replaced(host);
> +
> +		/*
> +		 * If bounced = false it means link rescanning (after probe
> +		 * or system resume), no link transition was recorded. The
> +		 * enumerated devices are intact, so remove them only if the
> +		 * connected device was replaced while link events were not
> +		 * delivered.
> +		 *
> +		 * If bounced = true it means at least one real link down/up
> +		 * cycle occurred.
> +		 */
> +		if (!bounced && !replaced)
> +			goto unlock;
> +
> +		rzg3s_pcie_remove_devices(bus);
> +	}
> +
> +	dev_info(host->dev, "PCIe link up, rescanning bus\n");
> +
> +	ret = rzg3s_pcie_set_max_link_speed(host);
> +	if (ret)
> +		dev_info(host->dev, "Failed to set max link speed\n");
> +
> +	pci_host_common_link_train_delay(host->max_link_speed);

[Severity: Medium]
When bounced is true and !list_empty(&bus->devices), does calling
rzg3s_pcie_device_replaced(host) issue Type 0 configuration reads to the
downstream endpoint before the link has finished settling?

When a real DL_UpDown event occurs (bounced == true), the result of
rzg3s_pcie_device_replaced(host) is not needed because the devices are
removed unconditionally, and rzg3s_pcie_set_max_link_speed() (which polls
for LTSSM L0 and negotiates link speed) and
pci_host_common_link_train_delay() have not been called yet. Should
rzg3s_pcie_device_replaced(host) only be called when !bounced?

[ ... ]

> @@ -1901,6 +2108,44 @@ static void rzv2h_pcie_release_lanes(void *data)
>  	rzv2h_num_total_lanes -= host->num_lanes;
>  }
>  
> +static int rzg3s_pcie_pm_notifier(struct notifier_block *nb,
> +				  unsigned long action, void *data)
> +{
> +	struct rzg3s_pcie_host *host = container_of(nb, struct rzg3s_pcie_host,
> +						    pm_nb);
> +
> +	switch (action) {
> +	case PM_SUSPEND_PREPARE:
> +		/* Disable link up/down interrupts. */
> +		if (host->started)
> +			writel_relaxed(0, host->axi + RZG3S_PCI_PEIE0);
> +		disable_irq(host->event_irq);
> +		break;

[Severity: High]
Can calling disable_irq(host->event_irq) inside rzg3s_pcie_pm_notifier()
deadlock on pm_chain_head.rwsem if rzg3s_pcie_event_irq_thread() is
concurrently probing or removing a PCI device whose driver registers or
unregisters a PM notifier?

During system suspend:

suspend_prepare()
  pm_notifier_call_chain_robust(PM_SUSPEND_PREPARE, PM_POST_SUSPEND)
    blocking_notifier_call_chain_robust(&pm_chain_head, ...)
      down_read(&pm_chain_head.rwsem)
      rzg3s_pcie_pm_notifier()
        disable_irq(host->event_irq)
          synchronize_irq(host->event_irq)

If rzg3s_pcie_event_irq_thread() is simultaneously running
rzg3s_pcie_link_event(), it calls pci_bus_add_devices() or
rzg3s_pcie_remove_devices() -> pci_stop_and_remove_bus_device(), which
synchronously runs PCI endpoint driver .probe() or .remove() callbacks.
Several PCI drivers (such as ath11k_pci, wil6210, amdgpu, and xe) call
register_pm_notifier() or unregister_pm_notifier() in those callbacks,
which blocks on down_write(&pm_chain_head.rwsem) while the suspend thread
holds down_read(&pm_chain_head.rwsem) waiting for the IRQ thread to finish.

> +
> +	case PM_POST_SUSPEND:
> +		/* Enable link up/down interrupts and force link re-scan. */
> +		if (host->started) {
> +			/*
> +			 * The link re-train in resume triggers the link up IRQ
> +			 * if there are connected devices. Clear the DL_UpDown
> +			 * status and trigger link_rescan to avoid
> +			 * re-enumerating the already connected devices.
> +			 */
> +			writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN,
> +				       host->axi + RZG3S_PCI_PEIS0);
> +			WRITE_ONCE(host->link_rescan, true);
> +			writel_relaxed(RZG3S_PCI_PEIE0_DL_UPDOWN,
> +				       host->axi + RZG3S_PCI_PEIE0);
> +		}
> +		enable_irq(host->event_irq);
> +		if (host->started)
> +			irq_wake_thread(host->event_irq, host);
> +		break;
> +	}
> +
> +	return NOTIFY_DONE;
> +}

[Severity: High]
rzg3s_pcie_pm_ops uses NOIRQ_SYSTEM_SLEEP_PM_OPS(rzg3s_pcie_suspend_noirq,
rzg3s_pcie_resume_noirq), which wires up the freeze_noirq, thaw_noirq,
poweroff_noirq, and restore_noirq callbacks for hibernation. However,
rzg3s_pcie_pm_notifier() only handles PM_SUSPEND_PREPARE and
PM_POST_SUSPEND.

During hibernation, won't omitting PM_HIBERNATION_PREPARE /
PM_RESTORE_PREPARE leave host->event_irq enabled during device freeze and
poweroff, and won't omitting PM_POST_HIBERNATION / PM_POST_RESTORE leave
RZG3S_PCI_PEIE0 cleared to 0 (after rzg3s_pcie_resume_noirq() ->
rzg3s_pcie_host_start() -> rzg3s_pcie_irq_init() resets it) and skip waking
the IRQ thread to rescan the bus?

>  
>  static int rzg3s_pcie_probe(struct platform_device *pdev)
>  {

[ ... ]

> @@ -1999,15 +2244,38 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto power_resets_assert;
>  
> +	host->started = true;
> +	host->pm_nb.notifier_call = rzg3s_pcie_pm_notifier;
> +	ret = register_pm_notifier(&host->pm_nb);
> +	if (ret)
> +		goto host_probe_teardown;
> +
>  	bridge->sysdata = host;
>  	bridge->ops = &rzg3s_pcie_root_ops;
>  	bridge->child_ops = &rzg3s_pcie_child_ops;
>  	ret = pci_host_probe(bridge);
>  	if (ret)
> -		goto host_probe_teardown;
> +		goto pm_notifier_unregister;
> +
> +	pci_lock_rescan_remove();
> +	rzg3s_pcie_cache_connected_dev_id(host);
> +	pci_unlock_rescan_remove();
> +
> +	/*
> +	 * The link re-train triggers the link up IRQ if there are connected
> +	 * devices. Clear DL_UpDown status and trigger link_rescan to avoid
> +	 * re-enumerating the devices detected while pci_host_probe() and loose
> +	 * any potential event.
> +	 */
> +	writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIS0);
> +	WRITE_ONCE(host->link_rescan, true);
> +	writel_relaxed(RZG3S_PCI_PEIE0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIE0);
> +	irq_wake_thread(host->event_irq, host);
>  
>  	return 0;

[Severity: High]
Because rzg3s_pcie_driver sets PROBE_PREFER_ASYNCHRONOUS, and system
suspend runs pm_notifier_call_chain_robust(PM_SUSPEND_PREPARE,
PM_POST_SUSPEND) before dpm_prepare() calls wait_for_device_probe(), can
rzg3s_pcie_probe() race with rzg3s_pcie_pm_notifier()?

1. host->started is set to true and register_pm_notifier(&host->pm_nb) is
called before pci_host_probe(bridge) initializes bridge->bus. If
suspend_prepare() aborts before pci_register_host_bridge() sets
bridge->bus, PM_POST_SUSPEND wakes rzg3s_pcie_event_irq_thread() ->
rzg3s_pcie_link_event(), which dereferences bridge->bus while it is still
NULL.

2. If PM_SUSPEND_PREPARE runs while rzg3s_pcie_probe() is inside
pci_host_probe(), rzg3s_pcie_pm_notifier() clears RZG3S_PCI_PEIE0 and calls
disable_irq(host->event_irq), but rzg3s_pcie_probe() then finishes before
wait_for_device_probe() returns and unconditionally enables
RZG3S_PCI_PEIE0_DL_UPDOWN and calls irq_wake_thread(host->event_irq, host),
waking the IRQ thread during suspend.

3. If PM_SUSPEND_PREPARE runs before register_pm_notifier(&host->pm_nb) is
called in rzg3s_pcie_probe(), PM_POST_SUSPEND will later call
enable_irq(host->event_irq) without a matching disable_irq().

[Severity: Medium]
In addition, clearing RZG3S_PCI_PEIS0_DL_UPDOWN here at the end of
rzg3s_pcie_probe() (after pci_host_probe(bridge)) and in
rzg3s_pcie_pm_notifier() under PM_POST_SUSPEND (after all device resume
callbacks or after an aborted suspend), rather than immediately after link
training in rzg3s_pcie_host_setup(), seems able to lose link bounce events:

If the same endpoint experiences a link down/up bounce during
pci_host_probe(), between rzg3s_pcie_resume_noirq() and PM_POST_SUSPEND, or
during an aborted suspend, clearing RZG3S_PCI_PEIS0_DL_UPDOWN discards the
hardware event so bounced is false. Because the same device is still
attached, rzg3s_pcie_device_replaced() returns false and
rzg3s_pcie_link_event() exits at:

	if (!bounced && !replaced)
		goto unlock;

leaving the endpoint with unconfigured BARs, PCI_COMMAND, and MSI state (or
at Gen1 speed if link-up raced with rzg3s_pcie_wait_for_link_up() timing
out in rzg3s_pcie_host_setup()). Should RZG3S_PCI_PEIS0_DL_UPDOWN be
cleared in rzg3s_pcie_host_setup() right after link training instead?

[ ... ]

> @@ -2041,8 +2309,25 @@ static int rzg3s_pcie_suspend_noirq(struct device *dev)
>  static int rzg3s_pcie_resume_noirq(struct device *dev)
>  {
>  	struct rzg3s_pcie_host *host = dev_get_drvdata(dev);
> +	int ret;
>  
> -	return rzg3s_pcie_host_start(host);
> +	ret = rzg3s_pcie_host_start(host);
> +	if (ret)
> +		return ret;
> +
> +	if (rzg3s_pcie_device_replaced(host)) {
> +		struct pci_host_bridge *bridge =
> +			pci_host_bridge_from_priv(host);
> +		struct pci_dev *port __free(pci_dev_put) =
> +			pci_get_slot(bridge->bus, PCI_DEVFN(0, 0));
> +
> +		if (port && port->subordinate) {
> +			pci_walk_bus(port->subordinate,
> +				     pci_dev_set_disconnected, NULL);
> +		}
> +	}
> +
> +	return 0;
>  }

[Severity: Medium]
If rzg3s_pcie_host_start(host) fails in rzg3s_pcie_resume_noirq(), its
error path asserts host->power_resets and clears
RZG3S_SYSC_FUNC_ID_RST_RSM_B, leaving host->started as false.

Although rzg3s_pcie_pm_notifier() checks host->started to avoid touching
controller registers after a failed resume, the PM core still runs
pci_pm_resume_noirq() and pci_pm_resume() on the child PCI devices. Since
rzg3s_pcie_resume_noirq() returns early before marking the devices
disconnected, and neither rzg3s_pcie_root_ops nor rzg3s_pcie_child_ops
checks host->started, won't config-space accesses during child device
resume (and later runtime accesses to the remaining PCI devices) still read
and write host->pcie and host->axi while the controller is held in reset?

Should the devices also be marked disconnected (or config accesses guarded
by host->started) when rzg3s_pcie_host_start() fails?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019065025.2146539-1-claudiu.beznea@kernel.org?part=13

Re: [PATCH v7 07/14] PCI: rzg3s-host: Use cached MSI address in .irq_compose_msi_msg()

[Lad, Prabhakar]

Hi Claudiu,

Thank you for the patch.

On Mon, Oct 19, 2026 at 7:57 AM Claudiu Beznea
<claudiu.beznea@kernel.org> wrote:
>
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The MSI window address is already cached in msi->window_base when the
> MSI window is configured. Use it to fill msg->address_lo and
> msg->address_hi in .irq_compose_msi_msg() instead of reading it back
> from the controller registers.
>
> This avoids unnecessary register accesses.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>
> Changes in v7:
> - none, this patch is new
>
>  drivers/pci/controller/pcie-rzg3s-host.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index b0a150262959..6acb0231a067 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
> @@ -660,19 +660,9 @@ static void rzg3s_pcie_irq_compose_msi_msg(struct irq_data *data,
>                                            struct msi_msg *msg)
>  {
>         struct rzg3s_pcie_msi *msi = irq_data_get_irq_chip_data(data);
> -       struct rzg3s_pcie_host *host = rzg3s_msi_to_host(msi);
> -       u32 lo, hi;
> -
> -       /*
> -        * Enable and msg data enable bits are part of the address lo. Drop
> -        * them along with the unused bit.
> -        */
> -       lo = readl_relaxed(host->axi + RZG3S_PCI_MSIRCVWADRL) &
> -            RZG3S_PCI_MSIRCVWADRL_MASK;
We need to drop RZG3S_PCI_MSIRCVWADRL_MASK macro, as this now triggers
the below warning:

drivers/pci/controller/pcie-rzg3s-host.c:66:9: warning: macro
‘RZG3S_PCI_MSIRCVWADRL_MASK’ is not used [-Wunused-macros]
   66 | #define RZG3S_PCI_MSIRCVWADRL_MASK              GENMASK(31, 3)

Cheers,
Prabhakar

> -       hi = readl_relaxed(host->axi + RZG3S_PCI_MSIRCVWADRU);
>
> -       msg->address_lo = lo;
> -       msg->address_hi = hi;
> +       msg->address_lo = lower_32_bits(msi->window_base);
> +       msg->address_hi = upper_32_bits(msi->window_base);
>         msg->data = data->hwirq;
>  }
>
> --
> 2.43.0
>
>

Re: [PATCH v7 07/14] PCI: rzg3s-host: Use cached MSI address in .irq_compose_msi_msg()

[Claudiu Beznea]

Hi, Prabhakar,

On 10/20/26 11:45, Lad, Prabhakar wrote:
> Hi Claudiu,
> 
> Thank you for the patch.
> 
> On Mon, Oct 19, 2026 at 7:57 AM Claudiu Beznea
> <claudiu.beznea@kernel.org> wrote:
>> From: Claudiu Beznea<claudiu.beznea.uj@bp.renesas.com>
>>
>> The MSI window address is already cached in msi->window_base when the
>> MSI window is configured. Use it to fill msg->address_lo and
>> msg->address_hi in .irq_compose_msi_msg() instead of reading it back
>> from the controller registers.
>>
>> This avoids unnecessary register accesses.
>>
>> Signed-off-by: Claudiu Beznea<claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v7:
>> - none, this patch is new
>>
>>   drivers/pci/controller/pcie-rzg3s-host.c | 14 ++------------
>>   1 file changed, 2 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
>> index b0a150262959..6acb0231a067 100644
>> --- a/drivers/pci/controller/pcie-rzg3s-host.c
>> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
>> @@ -660,19 +660,9 @@ static void rzg3s_pcie_irq_compose_msi_msg(struct irq_data *data,
>>                                             struct msi_msg *msg)
>>   {
>>          struct rzg3s_pcie_msi *msi = irq_data_get_irq_chip_data(data);
>> -       struct rzg3s_pcie_host *host = rzg3s_msi_to_host(msi);
>> -       u32 lo, hi;
>> -
>> -       /*
>> -        * Enable and msg data enable bits are part of the address lo. Drop
>> -        * them along with the unused bit.
>> -        */
>> -       lo = readl_relaxed(host->axi + RZG3S_PCI_MSIRCVWADRL) &
>> -            RZG3S_PCI_MSIRCVWADRL_MASK;
> We need to drop RZG3S_PCI_MSIRCVWADRL_MASK macro, as this now triggers
> the below warning:
> 
> drivers/pci/controller/pcie-rzg3s-host.c:66:9: warning: macro
> ‘RZG3S_PCI_MSIRCVWADRL_MASK’ is not used [-Wunused-macros]
>     66 | #define RZG3S_PCI_MSIRCVWADRL_MASK              GENMASK(31, 3)

Will update it in the next version.

Thank you for your review,
Claudiu