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

[Lad, Prabhakar]

Hi Geert,

On Mon, May 4, 2026 at 10:14 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> On Wed, 8 Apr 2026 at 20:55, Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> > On Wed, Mar 25, 2026 at 10:18 AM Claudiu Beznea
> > <claudiu.beznea@xxxxxxxxx> wrote:
> > > 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>
>
> > > > --- a/drivers/pci/controller/pcie-rzg3s-host.c
> > > > +++ b/drivers/pci/controller/pcie-rzg3s-host.c
> > > > @@ -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.
> > >
> > > 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>;
> > >
> > >         // ...
> > >     };
> > >
> > I'd like to get a clearer steer from the PCIe and DT maintainers
> > before investing further in either direction.
> >
> > To recap the two approaches on the table:
> >
> >   Option 1: A single renesas,pcie-controller-id property used to look up
> >             SYSC offsets in the driver.
> >
> >   Option 2: Explicit per-controller DT properties carrying the SYSC
> >             phandle, register offset, and mask for each functionality
> >             (L1 allow, mode, link-master, etc.).
> >
> > Both have trade-offs. Option 1 is simpler in the DT but moves hardware
> > knowledge into the driver, tightening the coupling. Option 2 is more
> > verbose but fully describes the hardware topology in the DT and avoids
> > a driver-internal lookup table.
>
> As this is SoC integration description, I think it belongs in DT.
> But adding many properties is indeed cumbersome.
> Fortunately the two register blocks inside SYSC seem to have the same
> layout for both channels, so you can just use a single property to
> refer to the base offsets (0x1000 and 0x1030).  Even if a difference
> would pop up later, you could check for e.g. == 0x1000 in the driver.
>
Okay, something like below?

&pcie0 {
  renesas,sysc = <&sysc 0x1000>;
};

&pcie1 {
  renesas,sysc = <&sysc 0x1030>;
};

> > Are there other approaches the maintainers would prefer that we
> > haven't considered?
>
> Option 3: Look at the unit address.
>
Indeed but this might be discouraged by DT maintainers ;)

Cheers,
Prabhakar