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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 7 Nov 2019 09:44:31 +0100
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Lad Prabhakar <prabhakar.csengg@...il.com>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Magnus Damm <magnus.damm@...il.com>,
        Kishon Vijay Abraham I <kishon@...com>,
        Marek Vasut <marek.vasut+renesas@...il.com>,
        Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
        linux-pci <linux-pci@...r.kernel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andrew Murray <andrew.murray@....com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        Chris Paterson <Chris.Paterson2@...esas.com>,
        "Lad, Prabhakar" <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH 3/5] PCI: rcar: Add R-Car PCIe endpoint device tree bindings

Hi Prabhakar,

On Wed, Nov 6, 2019 at 8:36 PM Lad Prabhakar <prabhakar.csengg@...il.com> wrote:
> From: "Lad, Prabhakar" <prabhakar.mahadev-lad.rj@...renesas.com>
>
> This patch adds the bindings for the R-Car PCIe endpoint driver.
>
> Signed-off-by: Lad, Prabhakar <prabhakar.mahadev-lad.rj@...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.

> +- 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?

> +- 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@...00000 {
> +               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 ;-)

> +               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@...ux-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