Hi, Manivannan,

Apologies for the late reply, I've been off for a while.

On 8/31/25 07:07, Manivannan Sadhasivam wrote:

On Sat, Aug 30, 2025 at 02:22:45PM GMT, Claudiu Beznea wrote:

quoted

On 30.08.2025 09:59, Manivannan Sadhasivam wrote:

quoted

On Fri, Jul 04, 2025 at 07:14:05PM GMT, Claudiu wrote:

quoted

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

The Renesas RZ/G3S features a PCIe IP that complies with the PCI Express
Base Specification 4.0 and supports speeds of up to 5 GT/s. It functions
only as a root complex, with a single-lane (x1) configuration. The
controller includes Type 1 configuration registers, as well as IP
specific registers (called AXI registers) required for various adjustments.

Hardware manual can be downloaded from the address in the "Link" section.
The following steps should be followed to access the manual:
1/ Click the "User Manual" button
2/ Click "Confirm"; this will start downloading an archive
3/ Open the downloaded archive
4/ Navigate to r01uh1014ej*-rzg3s-users-manual-hardware -> Deliverables
5/ Open the file r01uh1014ej*-rzg3s.pdf

Link: https://www.renesas.com/en/products/rz-g3s?queryID=695cc067c2d89e3f271d43656ede4d12
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

[...]

quoted

+static bool rzg3s_pcie_child_issue_request(struct rzg3s_pcie_host *host)
+{
+	u32 val;
+	int ret;
+
+	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_REQISS,
+			       RZG3S_PCI_REQISS_REQ_ISSUE,
+			       RZG3S_PCI_REQISS_REQ_ISSUE);
+	ret = readl_poll_timeout_atomic(host->axi + RZG3S_PCI_REQISS, val,
+					!(val & RZG3S_PCI_REQISS_REQ_ISSUE),
+					5, RZG3S_REQ_ISSUE_TIMEOUT_US);
+
+	return !!ret || (val & RZG3S_PCI_REQISS_MOR_STATUS);

You don't need to do !!ret as the C11 standard guarantees that any scalar type
stored as bool will have the value of 0 or 1.

OK, will drop it anyway as suggested in another thread.

quoted

quoted

+}
+

[...]

quoted

+static void __iomem *rzg3s_pcie_root_map_bus(struct pci_bus *bus,
+					     unsigned int devfn,
+					     int where)
+{
+	struct rzg3s_pcie_host *host = bus->sysdata;
+
+	if (devfn)
+		return NULL;

Is it really possible to have devfn as non-zero for a root bus?

I will drop it.

quoted

quoted

+
+	return host->pcie + where;
+}
+

[...]

quoted

+static int rzg3s_pcie_msi_setup(struct rzg3s_pcie_host *host)
+{
+	size_t size = RZG3S_PCI_MSI_INT_NR * sizeof(u32);
+	struct rzg3s_pcie_msi *msi = &host->msi;
+	struct device *dev = host->dev;
+	int id, ret;
+
+	msi->pages = __get_free_pages(GFP_KERNEL | GFP_DMA, 0);
+	if (!msi->pages)
+		return -ENOMEM;
+
+	msi->dma_addr = dma_map_single(dev, (void *)msi->pages, size * 2,
+				       DMA_BIDIRECTIONAL);
+	if (dma_mapping_error(dev, msi->dma_addr)) {
+		ret = -ENOMEM;
+		goto free_pages;
+	}
+
+	/*
+	 * According to the RZ/G3S HW manual (Rev.1.10, section 34.4.5.2 Setting
+	 * the MSI Window) the MSI window need to be within any AXI window. Find
+	 * an AXI window to setup the MSI window.

Are you really finding the AXI window or just making sure that the MSI window
falls into one of the AXI window?

I'm making sure the MSI windows falls into one of the enabled AXI windows.

Then you need to reword the comment as such. Currently, it is not clear.

OK

quoted

quoted

And I believe it is OK to have more than one MSI window within an AXI window.

This IP supports a single MSI window that need to fit into one of the
enabled AXI windows.

[...]

quoted

quoted

quoted

+
+	/* Update vendor ID and device ID */

Are you really updating it or setting it? If you are updating it, are the
default IDs invalid?

Default IDs are valid (at least on RZ/G3S) but Renesas specific. Renesas
wants to let individual users to set their own IDs.

So they are optional then? But the binding treats them as required, which should
be changed if the default IDs are valid.

On RZ/G3S the default IDs are valid. On other SoCs that will be using this
driver (e.g. RZ/G3E) the default IDs are not valid.

These were marked as required as Renesas wants them to be set by the
company that manufactures the end product itself.

quoted

quoted

quoted

+	writew(host->vendor_id, host->pcie + PCI_VENDOR_ID);
+	writew(host->device_id, host->pcie + PCI_DEVICE_ID);
+
+	/* HW manual recommends to write 0xffffffff on initialization */
+	writel(0xffffffff, host->pcie + RZG3S_PCI_CFG_BARMSK00L);
+	writel(0xffffffff, host->pcie + RZG3S_PCI_CFG_BARMSK00U);
+
+	/* Update bus info. */
+	writeb(primary_bus, host->pcie + PCI_PRIMARY_BUS);
+	writeb(secondary_bus, host->pcie + PCI_SECONDARY_BUS);
+	writeb(subordinate_bus, host->pcie + PCI_SUBORDINATE_BUS);
+
+	/* Disable access control to the CFGU */
+	writel(0, host->axi + RZG3S_PCI_PERM);
+
+	return 0;
+}
+

[...]

quoted

+static int rzg3s_pcie_host_init(struct rzg3s_pcie_host *host, bool probe)
+{
+	u32 val;
+	int ret;
+
+	/* Initialize the PCIe related registers */
+	ret = rzg3s_pcie_config_init(host);
+	if (ret)
+		return ret;
+
+	/* Initialize the interrupts */
+	rzg3s_pcie_irq_init(host);
+
+	ret = reset_control_bulk_deassert(host->data->num_cfg_resets,
+					  host->cfg_resets);
+	if (ret)
+		return ret;
+
+	/* 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,
+				 PCIE_LINK_WAIT_SLEEP_MS *
+				 PCIE_LINK_WAIT_MAX_RETRIES * MILLI);
+	if (ret) {
+		reset_control_bulk_assert(host->data->num_cfg_resets,
+					  host->cfg_resets);
+		return ret;
+	}
+
+	val = readl(host->axi + RZG3S_PCI_PCSTAT2);
+	dev_info(host->dev, "PCIe link status [0x%x]\n", val);
+
+	val = FIELD_GET(RZG3S_PCI_PCSTAT2_STATE_RX_DETECT, val);
+	dev_info(host->dev, "PCIe x%d: link up\n", hweight32(val));
+
+	if (probe) {
+		ret = devm_add_action_or_reset(host->dev,
+					       rzg3s_pcie_cfg_resets_action,
+					       host);

Oh well, this gets ugly. Now the devm_add_action_or_reset() is sprinkled
throughout the driver :/

As I said earlier, there are concerns in unloading the driver if it implements
an irqchip. So if you change the module_platform_driver() to
builtin_platform_driver() for this driver, these devm_add_action_or_reset()
calls become unused.

They can still be useful in case the probe fails. As the initialization
path is complicated, having actions or resets looks to me that makes the
code cleaner as the rest of devm_* helpers.

I can drop it and replace with gotos and dedicated functions but this will
complicate the code, AFAICT.

Please let me know how would you like me to proceed.

It is generally preferred to cleanup the resources in err path using goto
labels.

Just to be sure I'll prepare the next version with something that was
requested: would you like to have goto lables on this driver?

I kept it like this as I considered the code is simpler.

Thank you,
Claudiu