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
