Hi Claudiu,

Thanks for your review.

-----Original Message-----
From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
Sent: Thursday, February 26, 2026 12:24 PM
To: John Madieu <john.madieu.xa@bp.renesas.com>; Claudiu Beznea
[off-list ref]; lpieralisi@kernel.org;
kwilczynski@kernel.org; mani@kernel.org; geert+renesas@glider.be;
krzk+dt@kernel.org
Cc: robh@kernel.org; bhelgaas@google.com; conor+dt@kernel.org;
magnus.damm [off-list ref]; Biju Das
[off-list ref]; linux-pci@vger.kernel.org; linux-renesas-
soc@vger.kernel.org; devicetree@vger.kernel.org; linux-
clk@vger.kernel.org; john.madieu@gmail.com
Subject: Re: [PATCH v6 13/16] PCI: rzg3s-host: Add support for RZ/G3E
PCIe controller

Hi, John,

On 2/20/26 00:35, John Madieu wrote:

quoted

Add support for the PCIe controller found in RZ/G3E SoCs to the
existing RZ/G3S PCIe host driver. The RZ/G3E PCIe controller is
similar to the RZ/G3S's, with the following key differences:

  - Supports PCIe Gen3 (8.0 GT/s) link speeds alongside Gen2 (5.0

GT/s)

quoted

  - Uses a different reset control mechanism via AXI registers instead
    of the Linux reset framework
  - Requires specific SYSC configuration for link state control and
    Root Complex mode selection

Signed-off-by: John Madieu <john.madieu.xa@bp.renesas.com>
---

Changes:

v6:
   - Use rzg3s_sysc_config_func() with per-function calls instead of
     rzg3s_sysc_config() with -1 skip pattern, as suggested by Claudiu
   - Extend enum rzg3s_sysc_func_id with L1_ALLOW and MODE entries
   - Use regmap_update_bits() consistently for all SYSC accesses
   - Shorten comment to "Put controller in RC mode and de-assert

RST_RSM_B."

quoted

   - Drop "Enable ASPM L1 transition" comment (function ID is
self-documenting)

v5:
   - Introduce rzg3s_sysc_config() helper for sys configuration

v4: No changes
v3: No changes

  drivers/pci/controller/pcie-rzg3s-host.c | 98

++++++++++++++++++++++++

quoted

  1 file changed, 98 insertions(+)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c

b/drivers/pci/controller/pcie-rzg3s-host.c
index 413978feba1a..021b01746157 100644

--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c

@@ -111,6 +111,16 @@
  #define RZG3S_PCI_PERM_CFG_HWINIT_EN		BIT(2)
  #define RZG3S_PCI_PERM_PIPE_PHY_REG_EN		BIT(1)

+/* RZ/G3E specific registers */
+#define RZG3E_PCI_RESET				0x310
+#define RZG3E_PCI_RESET_RST_OUT_B		BIT(6)
+#define RZG3E_PCI_RESET_RST_PS_B		BIT(5)
+#define RZG3E_PCI_RESET_RST_LOAD_B		BIT(4)
+#define RZG3E_PCI_RESET_RST_CFG_B		BIT(3)
+#define RZG3E_PCI_RESET_RST_RSM_B		BIT(2)
+#define RZG3E_PCI_RESET_RST_GP_B		BIT(1)
+#define RZG3E_PCI_RESET_RST_B			BIT(0)

Currently, all the registers are prefixed with RZG3S. I know these
registers are RZG3E specific but the upcoming SoCs (RZ/V2H, RZ/T2N,
RZ/N2H) that will use this driver, have these registers implemented as
well. So, please keep these defines prefixed with RZG3S to have a
unified way across driver and let's keep only the SoC specific data
prefixed with SoC specific names.

To avoid any confusion, we could also drop the comment before these
register defines.

I'll switch to RZG3S_ prefix and remove the comments in v7.

quoted

+
  #define RZG3S_PCI_MSIRE(id)			(0x600 + (id) * 0x10)
  #define RZG3S_PCI_MSIRE_ENA			BIT(0)

@@ -184,10 +194,14 @@ struct rzg3s_sysc_function {
  /**
   * enum rzg3s_sysc_func_id - System controller function IDs
   * @RZG3S_SYSC_FUNC_ID_RST_RSM_B: RST_RSM_B SYSC function ID
+ * @RZG3S_SYSC_FUNC_ID_L1_ALLOW: L1 allow SYSC function ID
+ * @RZG3S_SYSC_FUNC_ID_MODE: Mode SYSC function ID
   * @RZG3S_SYSC_FUNC_ID_MAX: Max SYSC function ID
   */
  enum rzg3s_sysc_func_id {
  	RZG3S_SYSC_FUNC_ID_RST_RSM_B,
+	RZG3S_SYSC_FUNC_ID_L1_ALLOW,
+	RZG3S_SYSC_FUNC_ID_MODE,
  	RZG3S_SYSC_FUNC_ID_MAX,
  };

@@ -1135,6 +1149,49 @@ static int rzg3s_config_deinit(struct

rzg3s_pcie_host *host)

quoted

  					 host->cfg_resets);
  }

+/* RZ/G3E SoC-specific config implementations */

There is no similar comment on RZ/G3S SoC specific config
implementation. I would drop it as the function names and their usage
are meaningful enough.

Noted.

quoted

+static void rzg3e_pcie_config_pre_init(struct rzg3s_pcie_host *host)

This function is prefixed with rzg3e_pcie while the other ones on this
section are prefixed with rzg3e only. Please use a unified way of
defining them. I would go for rzg3e_pcie_config_{pre_init, deinit,
post_init}(). Also, please follow the same patter for the RZ/G3S ones,
e.g.
rzg3s_pcie_config_{post_init, deinit}(). Currently they are prefixed
only with the SoC name.

Will go with what you suggest.

quoted

+{
+	/*
+	 * De-assert LOAD_B and CFG_B during configuration phase.
+	 * These are part of the RZ/G3E reset register, not reset

framework.

quoted

+	 * Other reset bits remain asserted until config_post_init.
+	 */

I think this comment could be reduced to:

/* De-assert LOAD_B and CFG_B */

Statement "These are part of the RZ/G3E reset register, not reset
framework" has been mentioned in patch description already.

Also, about this "Other reset bits remain asserted until
config_post_init": this
  is the rationale for which the resets are handled in 3 different
functions, in
3 different steps, so, I think it can be dropped as well.

Will do.

quoted

+	rzg3s_pcie_update_bits(host->axi, RZG3E_PCI_RESET,
+			       RZG3E_PCI_RESET_RST_LOAD_B |

RZG3E_PCI_RESET_RST_CFG_B,

quoted

+			       RZG3E_PCI_RESET_RST_LOAD_B |

RZG3E_PCI_RESET_RST_CFG_B);

PCI subsystem is using 80 char lines. This function could be adjusted
like the above to follow this rule and keep the code more compact:

static void rzg3e_pcie_config_pre_init(struct rzg3s_pcie_host *host) {
	u32 mask = RZG3E_PCI_RESET_RST_LOAD_B | RZG3E_PCI_RESET_RST_CFG_B;

	/*
	 * De-assert LOAD_B and CFG_B during configuration phase.
	 * These are part of the RZ/G3E reset register, not reset
framework.
	 * Other reset bits remain asserted until config_post_init.
	 */
	rzg3s_pcie_update_bits(host->axi, RZG3E_PCI_RESET, mask, mask); }

quoted

+}

Noted.

quoted

+
+static int rzg3e_config_deinit(struct rzg3s_pcie_host *host) {
+	writel_relaxed(0, host->axi + RZG3E_PCI_RESET);
+	return 0;
+}
+
+static int rzg3e_config_post_init(struct rzg3s_pcie_host *host) {
+	/* De-assert PS_B, GP_B, RST_B */
+	rzg3s_pcie_update_bits(host->axi, RZG3E_PCI_RESET,
+			       RZG3E_PCI_RESET_RST_PS_B |

RZG3E_PCI_RESET_RST_GP_B |

quoted

+			       RZG3E_PCI_RESET_RST_B,
+			       RZG3E_PCI_RESET_RST_PS_B |

RZG3E_PCI_RESET_RST_GP_B |

quoted

+			       RZG3E_PCI_RESET_RST_B);

Same here, function could be written as:

static int rzg3e_config_post_init(struct rzg3s_pcie_host *host) {
	u32 mask = RZG3E_PCI_RESET_RST_PS_B | RZG3E_PCI_RESET_RST_GP_B |
		   RZG3E_PCI_RESET_RST_B;

	/* De-assert PS_B, GP_B, RST_B */
	rzg3s_pcie_update_bits(host->axi, RZG3E_PCI_RESET, mask, mask);

	/*
	 * According to the RZ/G3E HW manual (Rev.1.15, Table 6.6-130
	 * Initialization Procedure (RC)), hardware requires >= 500us delay
	 * before final reset deassert.
	 */
	fsleep(500);

	/* De-assert OUT_B and RSM_B to complete reset sequence */
	mask = RZG3E_PCI_RESET_RST_OUT_B | RZG3E_PCI_RESET_RST_RSM_B;
	rzg3s_pcie_update_bits(host->axi, RZG3E_PCI_RESET, mask, mask);

	return 0;
}

Noted.

quoted

+
+	/*
+	 * According to the RZ/G3E HW manual (Rev.1.15, Table 6.6-130
+	 * Initialization Procedure (RC)), hardware requires >= 500us delay
+	 * before final reset deassert.
+	 */
+	fsleep(500);
+
+	/* De-assert OUT_B and RSM_B to complete reset sequence */

/* De-assert OUT_B and RSM_B */

should be enough.

Noted.

quoted

+	rzg3s_pcie_update_bits(host->axi, RZG3E_PCI_RESET,
+			       RZG3E_PCI_RESET_RST_OUT_B |

RZG3E_PCI_RESET_RST_RSM_B,

quoted

+			       RZG3E_PCI_RESET_RST_OUT_B |

RZG3E_PCI_RESET_RST_RSM_B);

quoted

+
+	return 0;
+}
+
  static void rzg3s_pcie_irq_init(struct rzg3s_pcie_host *host)
  {
  	/*

@@ -1320,6 +1377,12 @@ static int rzg3s_pcie_host_init(struct

rzg3s_pcie_host *host)

quoted

  	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;

This should jump to config_deinit_and_refclk as
rzg3s_pcie_host_init_port() call above enables the port reference clock.

That's true. Will fix it in v7.

quoted

+
  	/* Initialize the interrupts */
  	rzg3s_pcie_irq_init(host);

@@ -1667,6 +1730,11 @@ static int rzg3s_pcie_probe(struct

platform_device *pdev)

quoted

  		goto port_refclk_put;
  	}

+	/* Put controller in RC mode and de-assert RST_RSM_B. */

/* Put controller in RC mode */

should be enough as this is what the above call is doing. The RST_RSM_B
is handled by the next rzg3s_sysc_config_func() call above and the macro
name for function ID is significant enough.

Noted.

quoted

+	ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_MODE, 1);
+	if (ret)
+		goto port_refclk_put;
+
  	ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B,

1);

quoted

  	if (ret)
  		goto port_refclk_put;

@@ -1781,6 +1849,10 @@ static int rzg3s_pcie_resume_noirq(struct

device *dev)

quoted

  	if (ret)
  		return ret;

+	ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_MODE, 1);
+	if (ret)
+		return ret;

This should jump to assert_rst_rsm_b;

But please keep the same order b/w MODE and RST_RSM_B setup as in probe
and there should be no need for jump.

Will do.

Regards,
John

Thank you,
Claudiu

quoted

+
  	ret = rzg3s_pcie_power_resets_deassert(host);
  	if (ret)
  		goto assert_rst_rsm_b;

@@ -1841,11 +1913,37 @@ static const struct rzg3s_pcie_soc_data

rzg3s_soc_data = {

quoted

  	},
  };

+static const char * const rzg3e_soc_power_resets[] = { "aresetn" };
+
+static const struct rzg3s_pcie_soc_data rzg3e_soc_data = {
+	.power_resets = rzg3e_soc_power_resets,
+	.num_power_resets = ARRAY_SIZE(rzg3e_soc_power_resets),
+	.config_pre_init = rzg3e_pcie_config_pre_init,
+	.config_post_init = rzg3e_config_post_init,
+	.config_deinit = rzg3e_config_deinit,
+	.sysc_info = {
+		.functions = {
+			[RZG3S_SYSC_FUNC_ID_L1_ALLOW] = {
+				.offset = 0x1020,
+				.mask = BIT(0),
+			},
+			[RZG3S_SYSC_FUNC_ID_MODE] = {
+				.offset = 0x1024,
+				.mask = BIT(0),
+			},
+		},
+	},
+};
+
  static const struct of_device_id rzg3s_pcie_of_match[] = {
  	{
  		.compatible = "renesas,r9a08g045-pcie",
  		.data = &rzg3s_soc_data,
  	},
+	{
+		.compatible = "renesas,r9a09g047-pcie",
+		.data = &rzg3e_soc_data,
+	},
  	{}
  };