Re: [PATCH v3 4/9] dt-bindings: PCI: renesas,r9a08g045s33-pcie: Add documentation for the PCIe IP on Renesas RZ/G3S

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

Hi, Bjorn,

On 08.07.2025 19:34, Bjorn Helgaas wrote:
> On Fri, Jul 04, 2025 at 07:14:04PM +0300, Claudiu wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> The PCIe IP available on the Renesas RZ/G3S complies with the PCI Express
>> Base Specification 4.0. It is designed for root complex applications and
>> features a single-lane (x1) implementation. Add documentation for it.
> 
>> +++ b/Documentation/devicetree/bindings/pci/renesas,r9a08g045s33-pcie.yaml
> 
> The "r9a08g045s33" in the filename seems oddly specific.  Does it
> leave room for descendants of the current chip that will inevitably be
> added in the future?  Most bindings are named with a fairly generic
> family name, e.g., "fsl,layerscape", "hisilicon,kirin", "intel,
> keembay", "samsung,exynos", etc.
> 
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/r9a08g045-cpg.h>
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +    bus {
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +
>> +        pcie@11e40000 {
>> +            compatible = "renesas,r9a08g045s33-pcie";
>> +            reg = <0 0x11e40000 0 0x10000>;
>> +            ranges = <0x02000000 0 0x30000000 0 0x30000000 0 0x8000000>;
>> +            dma-ranges = <0x42000000 0 0x48000000 0 0x48000000 0 0x38000000>;
>> +            bus-range = <0x0 0xff>;
>> +            clocks = <&cpg CPG_MOD R9A08G045_PCI_ACLK>,
>> +                     <&cpg CPG_MOD R9A08G045_PCI_CLKL1PM>;
>> +            clock-names = "aclk", "pm";
>> +            resets = <&cpg R9A08G045_PCI_ARESETN>,
>> +                     <&cpg R9A08G045_PCI_RST_B>,
>> +                     <&cpg R9A08G045_PCI_RST_GP_B>,
>> +                     <&cpg R9A08G045_PCI_RST_PS_B>,
>> +                     <&cpg R9A08G045_PCI_RST_RSM_B>,
>> +                     <&cpg R9A08G045_PCI_RST_CFG_B>,
>> +                     <&cpg R9A08G045_PCI_RST_LOAD_B>;
>> +            reset-names = "aresetn", "rst_b", "rst_gp_b", "rst_ps_b",
>> +                          "rst_rsm_b", "rst_cfg_b", "rst_load_b";
>> +            interrupts = <GIC_SPI 395 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 396 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 397 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 398 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 399 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 400 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 401 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 402 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 403 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 404 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 405 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 406 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 407 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 408 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 409 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 410 IRQ_TYPE_LEVEL_HIGH>;
>> +            interrupt-names = "serr", "serr_cor", "serr_nonfatal",
>> +                              "serr_fatal", "axi_err", "inta",
>> +                              "intb", "intc", "intd", "msi",
>> +                              "link_bandwidth", "pm_pme", "dma",
>> +                              "pcie_evt", "msg", "all";
>> +            #interrupt-cells = <1>;
>> +            interrupt-controller;
>> +            interrupt-map-mask = <0 0 0 7>;
>> +            interrupt-map = <0 0 0 1 &pcie 0 0 0 0>, /* INT A */
>> +                            <0 0 0 2 &pcie 0 0 0 1>, /* INT B */
>> +                            <0 0 0 3 &pcie 0 0 0 2>, /* INT C */
>> +                            <0 0 0 4 &pcie 0 0 0 3>; /* INT D */
> 
> The spec styles these closed up: "INTA", "INTB", etc.

I'll update it.

> 
>> +            device_type = "pci";
>> +            num-lanes = <1>;
>> +            #address-cells = <3>;
>> +            #size-cells = <2>;
>> +            power-domains = <&cpg>;
>> +            vendor-id = <0x1912>;
>> +            device-id = <0x0033>;
> 
> Some of this is specific to a Root Port, not to the Root Complex as a
> whole.  E.g., device-type = "pci", num-lanes, vendor-id, device-id,
> are Root Port properties.  Some of the resets, clocks, and interrupts
> might be as well.
> 
> I really want to separate those out because even though this
> particular version of this PCIe controller only supports a single Root
> Port, there are other controllers (and possibly future iterations of
> this controller) that support multiple Root Ports, and it makes
> maintenance easier if the DT bindings and the driver structures are
> similar.

I'll ask the Renesas HW team about the resets and clocks as the HW manual
don't offer any information about this.

If they will confirm some of the clocks and/or resets could be controlled
as part of a port then patch 3/9 "PCI: of_property: Restore the arguments
of the next level parent" in this series will not be needed anymore. Would
you prefer me to abandon it or post it as individual patch, if any?

> 
> This email includes pointers to sample DT bindings and driver code
> that is structured to allow multiple Root Ports:
> 
>   https://lore.kernel.org/linux-pci/20250625221653.GA1590146@bhelgaas/

Thank you for this!

And, thank you for your review,
Claudiu

> 
> Bjorn