Re: [PATCH 3/5] PCI: rcar: Add R-Car PCIe endpoint device tree bindings

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Thu, Nov 7, 2019 at 8:44 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> On Wed, Nov 6, 2019 at 8:36 PM Lad Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> > From: "Lad, Prabhakar" <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > This patch adds the bindings for the R-Car PCIe endpoint driver.
> >
> > Signed-off-by: Lad, Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> Thanks for your patch!
>
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/rcar-pci-ep.txt
> > @@ -0,0 +1,43 @@
> > +* Renesas R-Car PCIe Endpoint Controller DT description
> > +
> > +Required properties:
> > +           "renesas,pcie-ep-r8a774c0" for the R8A774C0 SoC;
> > +           "renesas,pcie-ep-rcar-gen3" for a generic R-Car Gen3 or
> > +                                    RZ/G2 compatible device.
>
> Unless I'm missing something, this is for the exact same hardware block as
> Documentation/devicetree/bindings/pci/rcar-pci.txt?
> So shouldn't you amend those bindings, instead of adding new compatible
> values?
> Please remember that DT describes hardware, not software policy.
> So IMHO choosing between host and endpoint is purely a configuration
> issue, and could be indicated by the presence or lack of some DT properties.
> E.g. host mode requires both "bus-range" and "device_type" properties,
> so their absence could indicate endpoint mode.
>
yes its the same hardware block as described in the rcar-pci.txt, I
did think about amending it
but  it might turn out to be bit messy,

required properties host ======required properties Endpoint
====================||==================
1: reg                                || reg
2:bus-range                      || reg names
3: device_type                  || resets
4: ranges                          || clocks
5: dma-ranges                  || clock-names
6: interrupts                      ||
7: interrupt-cells               ||
8: interrupt-map-mask     ||
9: clocks                          ||
10: clock-names             ||

and if I go ahead with the same compatible string that would mean to
add support for endpoint
mode in the host driver itself. I did follow the examples of
rockchip/cadence/designware where
its the same hardware block but has two different binding files one
for host mode and other for
endpoint mode.

> > +- reg: Five register ranges as listed in the reg-names property
> > +- reg-names: Must include the following names
> > +       - "apb-base"
> > +       - "memory0"
> > +       - "memory1"
> > +       - "memory2"
> > +       - "memory3"
>
> What is the purpose of the last 4 regions?
> Can they be chosen by the driver, at runtime?
>
no the driver cannot choose them at runtime, as these are the only
PCIE memory(0/1/2/3) ranges
in the AXI address space where host memory can be mapped.

> > +- resets: Must contain phandles to PCIe-related reset lines exposed by IP block
> > +- clocks: from common clock binding: clock specifiers for the PCIe controller
> > +        clock.
> > +- clock-names: from common clock binding: should be "pcie".
> > +
> > +Optional Property:
> > +- max-functions: Maximum number of functions that can be configured (default 1).
> > +
> > +Example:
> > +
> > +SoC-specific DT Entry:
> > +
> > +       pcie_ep: pcie_ep@fe000000 {
> > +               compatible = "renesas,pcie-r8a7791", "renesas,pcie-rcar-gen2";
>
> These compatible values do not match with the ones above
> (but they match with what I'd like to see ;-)
>
my bad I'll update them to reflect the above.

Cheers,
--Prabhakar

> > +               reg = <0 0xfe000000 0 0x80000>,
> > +                       <0x0 0xfe100000 0 0x100000>,
> > +                       <0x0 0xfe200000 0 0x200000>,
> > +                       <0x0 0x30000000 0 0x8000000>,
> > +                       <0x0 0x38000000 0 0x8000000>;
> > +               reg-names = "apb-base", "memory0", "memory1", "memory2", "memory3";
> > +               clocks = <&cpg CPG_MOD 319>;
> > +               clock-names = "pcie";
> > +               power-domains = <&sysc R8A774C0_PD_ALWAYS_ON>;
> > +               resets = <&cpg 319>;
> > +       };
>
> 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