Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191106193609.19645-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
 <20191106193609.19645-4-prabhakar.mahadev-lad.rj@bp.renesas.com> <CAMuHMdVZwgVnq2kwjNJQHfvUH0sk6M7Hz-AJR82jMOsCNfW9wQ@mail.gmail.com>
In-Reply-To: <CAMuHMdVZwgVnq2kwjNJQHfvUH0sk6M7Hz-AJR82jMOsCNfW9wQ@mail.gmail.com>
From:   "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
Date:   Thu, 7 Nov 2019 09:25:42 +0000
Message-ID: <CA+V-a8swtOUaxKnCdiTV5wvvxLEJ6XdODL=7bvQmFKY0zQTj2w@mail.gmail.com>
Subject: Re: [PATCH 3/5] PCI: rcar: Add R-Car PCIe endpoint device tree bindings
To:     Geert Uytterhoeven <geert@linux-m68k.org>
Cc:     Bjorn Helgaas <bhelgaas@google.com>,
        Rob Herring <robh+dt@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        Magnus Damm <magnus.damm@gmail.com>,
        Kishon Vijay Abraham I <kishon@ti.com>,
        Marek Vasut <marek.vasut+renesas@gmail.com>,
        Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
        linux-pci <linux-pci@vger.kernel.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Andrew Murray <andrew.murray@arm.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
        Chris Paterson <Chris.Paterson2@renesas.com>,
        "Lad, Prabhakar" <prabhakar.mahadev-lad.rj@bp.renesas.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Geert,

Thank you for the review.

On Thu, Nov 7, 2019 at 8:44 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Wed, Nov 6, 2019 at 8:36 PM Lad Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: "Lad, Prabhakar" <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > This patch adds the bindings for the R-Car PCIe endpoint driver.
> >
> > Signed-off-by: Lad, Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> 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@linux-m68k.org
>
> 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