Re: [PATCH 5/5] PCI: rzg3s-host: Add support for RZ/V2H(P) SoC

[Lad, Prabhakar]

Hi Claudiu,

Thank you for the review.

On Wed, Mar 25, 2026 at 10:18 AM Claudiu Beznea
<claudiu.beznea@xxxxxxxxx> wrote:
>
> Hi, Prabhakar,
>
> On 3/18/26 14:44, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxx>
> > ---
> >   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
>
> > +     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?"
>
> > +
> > +     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