Hi Claudiu,

Thank you for the review.

On Wed, Mar 25, 2026 at 10:18 AM Claudiu Beznea
[off-list ref] wrote:

Hi, Prabhakar,

On 3/18/26 14:44, Prabhakar wrote:

quoted

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Add support for the RZ/V2H(P) SoC PCIe controller to the rzg3s-host
driver.

The RZ/V2H(P) SoC features two independent PCIe channels that share
physical lanes. The hardware supports two configuration modes: single
x4 mode where one controller uses all four lanes, or dual x2 mode
where both controllers use two lanes each.

Introduce configure_lanes() function pointer to configure the PCIe
lanes based on the number of channels enabled. Implement
rzv2h_pcie_configure_lanes() to detect the active PCIe channels at
boot time and program the lane mode via the system controller using
the new RZG3S_SYSC_FUNC_ID_LINK_MASTER function ID.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
  drivers/pci/controller/pcie-rzg3s-host.c | 142 +++++++++++++++++++++++
  1 file changed, 142 insertions(+)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index a629e861bbd0..d1bf1e750d9b 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c

@@ -179,6 +179,16 @@
  /* Timeouts experimentally determined */
  #define RZG3S_REQ_ISSUE_TIMEOUT_US          2500

+/**
+ * enum rzg3s_sysc_link_mode - PCIe link configuration modes
+ * @RZG3S_SYSC_LINK_MODE_SINGLE_X4: Single port with x4 lanes
+ * @RZG3S_SYSC_LINK_MODE_DUAL_X2: Dual ports with x2 lanes each
+ */
+enum rzg3s_sysc_link_mode {
+     RZG3S_SYSC_LINK_MODE_SINGLE_X4 = 1,
+     RZG3S_SYSC_LINK_MODE_DUAL_X2 = 3,
+};
+
  /**
   * struct rzg3s_sysc_function - System Controller function descriptor
   * @offset: Register offset from the System Controller base address

@@ -194,12 +204,14 @@ struct rzg3s_sysc_function {
   * @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_LINK_MASTER: Link master 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_LINK_MASTER,
      RZG3S_SYSC_FUNC_ID_MAX,
  };

@@ -261,6 +273,7 @@ struct rzg3s_pcie_host;
   * @config_pre_init: Optional callback for SoC-specific pre-configuration
   * @config_post_init: Callback for SoC-specific post-configuration
   * @config_deinit: Callback for SoC-specific de-initialization
+ * @setup_lanes: Callback for setting up the number of lanes
   * @power_resets: array with the resets that need to be de-asserted after
   *                power-on
   * @cfg_resets: array with the resets that need to be de-asserted after

@@ -268,17 +281,20 @@ struct rzg3s_pcie_host;
   * @sysc_info: System Controller info for each PCIe channel
   * @num_power_resets: number of power resets
   * @num_cfg_resets: number of configuration resets
+ * @num_channels: number of PCIe channels
   */
  struct rzg3s_pcie_soc_data {
      int (*init_phy)(struct rzg3s_pcie_host *host);
      void (*config_pre_init)(struct rzg3s_pcie_host *host);
      int (*config_post_init)(struct rzg3s_pcie_host *host);
      int (*config_deinit)(struct rzg3s_pcie_host *host);
+     int (*setup_lanes)(struct rzg3s_pcie_host *host);
      const char * const *power_resets;
      const char * const *cfg_resets;
      struct rzg3s_sysc_info sysc_info[RZG3S_PCIE_CHANNEL_ID_MAX];
      u8 num_power_resets;
      u8 num_cfg_resets;
+     u8 num_channels;
  };

  /**

@@ -309,6 +325,7 @@ struct rzg3s_pcie_port {
   * @intx_irqs: INTx interrupts
   * @max_link_speed: maximum supported link speed
   * @channel_id: PCIe channel identifier, used for System Controller access
+ * @num_lanes: The number of lanes
   */
  struct rzg3s_pcie_host {
      void __iomem *axi;

@@ -325,6 +342,7 @@ struct rzg3s_pcie_host {
      int intx_irqs[PCI_NUM_INTX];
      int max_link_speed;
      enum rzg3s_pcie_channel_id channel_id;
+     u8 num_lanes;
  };

  #define rzg3s_msi_to_host(_msi)     container_of(_msi, struct rzg3s_pcie_host, msi)

@@ -1155,6 +1173,13 @@ static int rzg3s_pcie_config_init(struct rzg3s_pcie_host *host)
      rzg3s_pcie_update_bits(host->pcie, PCI_CLASS_REVISION, mask,
                             field_prep(mask, PCI_CLASS_BRIDGE_PCI_NORMAL));

+     if (host->num_lanes) {
+             rzg3s_pcie_update_bits(host->pcie + RZG3S_PCI_CFG_PCIEC,
+                                    PCI_EXP_LNKCAP, PCI_EXP_LNKCAP_MLW,
+                                    FIELD_PREP(PCI_EXP_LNKCAP_MLW,
+                                               host->num_lanes));
+     }
+
      /* Disable access control to the CFGU */
      writel_relaxed(0, host->axi + RZG3S_PCI_PERM);

@@ -1687,6 +1712,63 @@ rzg3s_pcie_host_setup(struct rzg3s_pcie_host *host,
      return ret;
  }

+static int rzg3s_pcie_get_controller_id(struct rzg3s_pcie_host *host)
+{
+     struct device_node *np = host->dev->of_node;
+     u32 domain;
+     int ret;
+
+     if (host->data->num_channels == 1)
+             return 0;
+
+     ret = of_property_read_u32(np, "linux,pci-domain", &domain);

This introduces some limits in the systems with RZ/V2H(P) SoCs with regards to
the usage of linux,pci-domain. I would like the PCIe maintainers take on this.

+ DT maintainers too.

As this is necessary to index in the system controller driver specific data (as
there are different SYSC offsets for different PCIe controllers) I see the
following alternatives, if any:

1/ add a dedicated DT property for this, e.g. renesas,pcie-controller-id
2/ Add dedicated DT bindings for RZ/V2H(P) SoC that would be used to specify the
    system controller register offset and mask for different functionalities.

    E.g.:
    renesas,sysc-l1-allow = <&sysc 0x1020 0x1>;
    renesas,sysc-mode = <&sysc 0x1024 0x1>;
    renesas,sysc-link-master = <&sysc 0x1060 0x300>;

    And use them in each controller DT node. E.g.:

    pcie0: pcie@add1 {
        // ...

        renesas,sysc-l1-allow = <&sysc 0x1020 0x1>;
        renesas,sysc-mode = <&sysc 0x1024 0x1>;
        renesas,sysc-link-master = <&sysc 0x1060 0x300>;

        // ...
    };

    pcie0: pcie@add1 {
        // ...

        renesas,sysc-l1-allow = <&sysc 0x1050 0x1>;
        renesas,sysc-mode = <&sysc 0x1054 0x1>;
        renesas,sysc-link-master = <&sysc 0x1060 0x300>;

        // ...
    };

The current approach is being used to align with the existing driver
design and to reuse the already available DT property, rather than
introducing a new one.

Regarding option #2, I don’t see this as a scalable solution. For
every new register, we would need to introduce a separate DT property,
which would quickly become unwieldy and harder to maintain.

Im ok, with option #1 or any other suggestion based on feedback of
PCIe and DT maintainers.

3/ as sashiko.dev mentions [1], using aliases for the PCIe nodes should also be
    what you need here.

[1]
https://sashiko.dev/#/patchset/20260318124450.163471-1-prabhakar.mahadev-lad.rj%40bp.renesas.com

quoted

+     if (ret)
+             return ret;
+
+     if (domain >= host->data->num_channels)
+             return -EINVAL;
+
+     host->channel_id = domain;
+
+     return 0;
+}
+
+static int rzv2h_pcie_setup_lanes(struct rzg3s_pcie_host *host)
+{
+     struct device_node *np = host->dev->of_node;
+     static u8 rzv2h_num_total_lanes;
+     u32 num_lanes;
+     int ret;
+
+     ret = of_property_read_u32(np, "num-lanes", &num_lanes);
+     if (ret)
+             return ret;
+
+     /*
+      * RZ/V2H(P) supports up to 4 lanes, but only in single x4 mode.
+      * Dual x2 mode is only supported with 2 total lanes. Validate
+      * the configuration to avoid conflicts with other host, if any.
+      */
+     if (num_lanes != 4 && num_lanes != 2)
+             return -EINVAL;
+
+     if (rzv2h_num_total_lanes == 2 && num_lanes != 2)
+             return -EINVAL;
+
+     if (rzv2h_num_total_lanes == 4)
+             return -EINVAL;
+
+     rzv2h_num_total_lanes += num_lanes;

There is a a valid concern raised by sashiko.dev [1] with regards to
incrementing this if later the probe fails:

from [1]:
"For example, if rzg3s_pcie_resets_prepare_and_get() returns -EPROBE_DEFER,
the static variable is never decremented. On subsequent probe retries,
the variable will be artificially inflated, eventually causing the bounds
check to fail and returning a permanent -EINVAL. This would also prevent
driver unbind and rebind from working correctly."

The other alternative would be the below, where we wouldn't need to
use the num-lanes property but would need a comparison with the DT
compatible,

+       for_each_compatible_node(np, NULL, "renesas,r9a09g057-pcie") {
+               if (of_device_is_available(np))
+                       count++;
+       }
+       if (!count)
+               return 0;
+
+       /* If both PCIe channels are enabled configure the LINK_MASTER
in x2 lane mode.
+        * If only one channel is enabled check the port index and if
port1 is enabled
+        * configure the LINK_MASTER in x2 lane mode, otherwise keep
it in x4 lane mode.
+        */
+       if (count == RZV2H_MAX_PCIE_PORTS ||
+           (count == 1 && host->channel == 1))
+               host->link_mode = RZV2H_PCIE_MODE_DUAL_X2;
+       else
+               host->link_mode = RZV2H_PCIE_MODE_SINGLE_X4;

also:

"Additionally, since the driver sets .probe_type = PROBE_PREFER_ASYNCHRONOUS,
could multiple PCIe controllers probing concurrently cause a data race when
reading and modifying this static variable without locking?"

quoted

+
+     host->num_lanes = num_lanes;
+
+     return rzg3s_sysc_config_func(host->sysc,
+                                   RZG3S_SYSC_FUNC_ID_LINK_MASTER,
+                                   num_lanes == 2 ?
+                                   RZG3S_SYSC_LINK_MODE_DUAL_X2 :
+                                   RZG3S_SYSC_LINK_MODE_SINGLE_X4);

I think this one should also be configured on resume (to have the same
configuration sequence as in probe) even though RZ/V2H(P) don't currently
support s2ram. E.g. so something like:

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;
}

after ret = rzg3s_sysc_config_func(sysc, RZG3S_SYSC_FUNC_ID_RST_RSM_B, 1);

Ok.

Cheers,
Prabhakar