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

[Geert Uytterhoeven]

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.

> Are there other approaches the maintainers would prefer that we
> haven't considered?

Option 3: Look at the unit address.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds