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

Commit Message

Lad, Prabhakar Nov. 6, 2019, 7:36 p.m. UTC

From: "Lad, Prabhakar" <[e-mail]>

This patch adds the bindings for the R-Car PCIe endpoint driver.

Signed-off-by: Lad, Prabhakar <[e-mail]>
---
 .../devicetree/bindings/pci/rcar-pci-ep.txt   | 43 +++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/rcar-pci-ep.txt

Comments

Biju Das Nov. 7, 2019, 7:39 a.m. UTC | #1

Hi Prabhakar,

Thanks for the patch

> Subject: [PATCH 3/5] PCI: rcar: Add R-Car PCIe endpoint device tree bindings
> 
> From: "Lad, Prabhakar" <[e-mail]>
> 
> This patch adds the bindings for the R-Car PCIe endpoint driver.
> 
> Signed-off-by: Lad, Prabhakar <[e-mail]>
> ---
>  .../devicetree/bindings/pci/rcar-pci-ep.txt   | 43 +++++++++++++++++++
>  1 file changed, 43 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/rcar-pci-ep.txt
> 
> diff --git a/Documentation/devicetree/bindings/pci/rcar-pci-ep.txt
> b/Documentation/devicetree/bindings/pci/rcar-pci-ep.txt
> new file mode 100644
> index 000000000000..b8c8616ca007
> --- /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.
> +
> +	    When compatible with the generic version, nodes must list the
> +	    SoC-specific version corresponding to the platform first
> +	    followed by the generic version.
> +
> +- reg: Five register ranges as listed in the reg-names property
> +- reg-names: Must include the following names
> +	- "apb-base"
> +	- "memory0"
> +	- "memory1"
> +	- "memory2"
> +	- "memory3"
> +- 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";

I believe it is currently tested on RZ/G2E. so please use the same.

Cheers,
Biju

> +		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>;
> +	};
> --
> 2.20.1

Lad, Prabhakar Nov. 7, 2019, 8:10 a.m. UTC | #2

Hi Biju,

Thank you for the review.

On Thu, Nov 7, 2019 at 7:39 AM Biju Das <[e-mail]> wrote:
>
> Hi Prabhakar,
>
> Thanks for the patch
>
> > Subject: [PATCH 3/5] PCI: rcar: Add R-Car PCIe endpoint device tree bindings
> >
> > From: "Lad, Prabhakar" <[e-mail]>
> >
> > This patch adds the bindings for the R-Car PCIe endpoint driver.
> >
> > Signed-off-by: Lad, Prabhakar <[e-mail]>
> > ---
> >  .../devicetree/bindings/pci/rcar-pci-ep.txt   | 43 +++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pci/rcar-pci-ep.txt
> >
> > diff --git a/Documentation/devicetree/bindings/pci/rcar-pci-ep.txt
> > b/Documentation/devicetree/bindings/pci/rcar-pci-ep.txt
> > new file mode 100644
> > index 000000000000..b8c8616ca007
> > --- /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.
> > +
> > +         When compatible with the generic version, nodes must list the
> > +         SoC-specific version corresponding to the platform first
> > +         followed by the generic version.
> > +
> > +- reg: Five register ranges as listed in the reg-names property
> > +- reg-names: Must include the following names
> > +     - "apb-base"
> > +     - "memory0"
> > +     - "memory1"
> > +     - "memory2"
> > +     - "memory3"
> > +- 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";
>
> I believe it is currently tested on RZ/G2E. so please use the same.
>
Yes you are correct its tested on RZ/G2E board, ill fix it in next iteration.

Cheers,
--Prabhakar

Geert Uytterhoeven Nov. 7, 2019, 8:44 a.m. UTC | #3

Hi Prabhakar,

On Wed, Nov 6, 2019 at 8:36 PM Lad Prabhakar <[e-mail]> wrote:
> From: "Lad, Prabhakar" <[e-mail]>
>
> This patch adds the bindings for the R-Car PCIe endpoint driver.
>
> Signed-off-by: Lad, Prabhakar <[e-mail]>

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@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 ;-)

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

Lad, Prabhakar Nov. 7, 2019, 9:25 a.m. UTC | #4

Hi Geert,

Thank you for the review.

On Thu, Nov 7, 2019 at 8:44 AM Geert Uytterhoeven <[e-mail]> wrote:
>
> Hi Prabhakar,
>
> On Wed, Nov 6, 2019 at 8:36 PM Lad Prabhakar <[e-mail]> wrote:
> > From: "Lad, Prabhakar" <[e-mail]>
> >
> > This patch adds the bindings for the R-Car PCIe endpoint driver.
> >
> > Signed-off-by: Lad, Prabhakar <[e-mail]>
>
> 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 -- [e-mail]
>
> 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

Geert Uytterhoeven Nov. 7, 2019, 8:08 p.m. UTC | #5

Hi Prabhakar,

On Thu, Nov 7, 2019 at 10:26 AM Lad, Prabhakar
<[e-mail]> wrote:
> On Thu, Nov 7, 2019 at 8:44 AM Geert Uytterhoeven <[e-mail]> wrote:
> > On Wed, Nov 6, 2019 at 8:36 PM Lad Prabhakar <[e-mail]> wrote:
> > > From: "Lad, Prabhakar" <[e-mail]>
> > >
> > > This patch adds the bindings for the R-Car PCIe endpoint driver.
> > >
> > > Signed-off-by: Lad, Prabhakar <[e-mail]>
> >
> > 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             ||

We have a similar situation with SPI, where a controller can operate in
master or slave mode, based on the absence or presence of the
"spi-slave" DT property.

> 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

You can still have two separate drivers, binding against the same
compatible value.  Just let the .probe() function return -ENODEV if it
discovers (by looking at DT properties) if the node is configured for
the other mode.
Which brings us to my next questions: is there any code that could be
shared between the drivers for the two modes?

> rockchip/cadence/designware where
> its the same hardware block but has two different binding files one
> for host mode and other for
> endpoint mode.

Having two separate DT binding documents sounds fine to me, if unifying
them makes things too complex.
However, I think they should use the same compatible value, because the
hardware block is the same, but just used in a different mode.

Rob/Mark: Any input from the DT maintainers?

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

Are they fixed by the PCIe hardware, i.e. could they be looked up by the
driver based on the compatible value?

Gr{oetje,eeting}s,

                        Geert

Lad, Prabhakar Nov. 7, 2019, 10:46 p.m. UTC | #6

Hi Geert,

On Thu, Nov 7, 2019 at 8:08 PM Geert Uytterhoeven <[e-mail]> wrote:
>
> Hi Prabhakar,
>
> On Thu, Nov 7, 2019 at 10:26 AM Lad, Prabhakar
> <[e-mail]> wrote:
> > On Thu, Nov 7, 2019 at 8:44 AM Geert Uytterhoeven <[e-mail]> wrote:
> > > On Wed, Nov 6, 2019 at 8:36 PM Lad Prabhakar <[e-mail]> wrote:
> > > > From: "Lad, Prabhakar" <[e-mail]>
> > > >
> > > > This patch adds the bindings for the R-Car PCIe endpoint driver.
> > > >
> > > > Signed-off-by: Lad, Prabhakar <[e-mail]>
> > >
> > > 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             ||
>
> We have a similar situation with SPI, where a controller can operate in
> master or slave mode, based on the absence or presence of the
> "spi-slave" DT property.
>
> > 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
>
> You can still have two separate drivers, binding against the same
> compatible value.  Just let the .probe() function return -ENODEV if it
> discovers (by looking at DT properties) if the node is configured for
> the other mode.
> Which brings us to my next questions: is there any code that could be
> shared between the drivers for the two modes?
>
agreed driver probe could handle depending on the DT properties.
yes there is bit of common code and the first patch of the series prepares
for handling host and endpoint mode.

> > rockchip/cadence/designware where
> > its the same hardware block but has two different binding files one
> > for host mode and other for
> > endpoint mode.
>
> Having two separate DT binding documents sounds fine to me, if unifying
> them makes things too complex.
> However, I think they should use the same compatible value, because the
> hardware block is the same, but just used in a different mode.
>
agreed.

> Rob/Mark: Any input from the DT maintainers?
>
> > > > +- 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.
>
> Are they fixed by the PCIe hardware, i.e. could they be looked up by the
> driver based on the compatible value?
>
yes they are fixed by the PCIe hardware and could be looked up by the driver
based on the compatible value.

Cheers,
--Prabhakar

Geert Uytterhoeven Nov. 8, 2019, 8:36 a.m. UTC | #7

Hi Prabhakar,

On Thu, Nov 7, 2019 at 11:46 PM Lad, Prabhakar
<[e-mail]> wrote:
> On Thu, Nov 7, 2019 at 8:08 PM Geert Uytterhoeven <[e-mail]> wrote:
> > On Thu, Nov 7, 2019 at 10:26 AM Lad, Prabhakar
> > <[e-mail]> wrote:
> > > On Thu, Nov 7, 2019 at 8:44 AM Geert Uytterhoeven <[e-mail]> wrote:
> > > > On Wed, Nov 6, 2019 at 8:36 PM Lad Prabhakar <[e-mail]> wrote:
> > > > > From: "Lad, Prabhakar" <[e-mail]>
> > > > >
> > > > > This patch adds the bindings for the R-Car PCIe endpoint driver.
> > > > >
> > > > > Signed-off-by: Lad, Prabhakar <[e-mail]>
> > > >
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/pci/rcar-pci-ep.txt

> > > > > +- 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.
> >
> > Are they fixed by the PCIe hardware, i.e. could they be looked up by the
> > driver based on the compatible value?
> >
> yes they are fixed by the PCIe hardware and could be looked up by the driver
> based on the compatible value.

Thanks, so we don't need to describe them in DT.

Gr{oetje,eeting}s,

                        Geert

Rob Herring (Arm) Nov. 13, 2019, 4:08 a.m. UTC | #8

On Thu, Nov 07, 2019 at 09:08:35PM +0100, Geert Uytterhoeven wrote:
> Hi Prabhakar,
> 
> On Thu, Nov 7, 2019 at 10:26 AM Lad, Prabhakar
> <[e-mail]> wrote:
> > On Thu, Nov 7, 2019 at 8:44 AM Geert Uytterhoeven <[e-mail]> wrote:
> > > On Wed, Nov 6, 2019 at 8:36 PM Lad Prabhakar <[e-mail]> wrote:
> > > > From: "Lad, Prabhakar" <[e-mail]>
> > > >
> > > > This patch adds the bindings for the R-Car PCIe endpoint driver.
> > > >
> > > > Signed-off-by: Lad, Prabhakar <[e-mail]>
> > >
> > > 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             ||
> 
> We have a similar situation with SPI, where a controller can operate in
> master or slave mode, based on the absence or presence of the
> "spi-slave" DT property.
> 
> > 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
> 
> You can still have two separate drivers, binding against the same
> compatible value.  Just let the .probe() function return -ENODEV if it
> discovers (by looking at DT properties) if the node is configured for
> the other mode.
> Which brings us to my next questions: is there any code that could be
> shared between the drivers for the two modes?
> 
> > rockchip/cadence/designware where
> > its the same hardware block but has two different binding files one
> > for host mode and other for
> > endpoint mode.
> 
> Having two separate DT binding documents sounds fine to me, if unifying
> them makes things too complex.
> However, I think they should use the same compatible value, because the
> hardware block is the same, but just used in a different mode.
> 
> Rob/Mark: Any input from the DT maintainers?

Separate files makes sense because different modes will want to 
include different common schemas. We've generally been doing different 
compatibles too which makes validating the node has the right set of 
properties easier.
 
> > > > +- 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.
> 
> Are they fixed by the PCIe hardware, i.e. could they be looked up by the
> driver based on the compatible value?

That would be strange for a memory range.

Sounds like like 'ranges' though I'm not sure if 'ranges' for an EP 
makes sense or what that should look like.

Rob

Kishon Vijay Abraham I Nov. 27, 2019, 5:44 a.m. UTC | #9

Hi,

On 13/11/19 9:38 AM, Rob Herring wrote:
> On Thu, Nov 07, 2019 at 09:08:35PM +0100, Geert Uytterhoeven wrote:
>> Hi Prabhakar,
>>
>> On Thu, Nov 7, 2019 at 10:26 AM Lad, Prabhakar
>> <[e-mail]> wrote:
>>> On Thu, Nov 7, 2019 at 8:44 AM Geert Uytterhoeven <[e-mail]> wrote:
>>>> On Wed, Nov 6, 2019 at 8:36 PM Lad Prabhakar <[e-mail]> wrote:
>>>>> From: "Lad, Prabhakar" <[e-mail]>
>>>>>
>>>>> This patch adds the bindings for the R-Car PCIe endpoint driver.
>>>>>
>>>>> Signed-off-by: Lad, Prabhakar <[e-mail]>
>>>>
>>>> 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             ||
>>
>> We have a similar situation with SPI, where a controller can operate in
>> master or slave mode, based on the absence or presence of the
>> "spi-slave" DT property.
>>
>>> 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
>>
>> You can still have two separate drivers, binding against the same
>> compatible value.  Just let the .probe() function return -ENODEV if it
>> discovers (by looking at DT properties) if the node is configured for
>> the other mode.
>> Which brings us to my next questions: is there any code that could be
>> shared between the drivers for the two modes?
>>
>>> rockchip/cadence/designware where
>>> its the same hardware block but has two different binding files one
>>> for host mode and other for
>>> endpoint mode.
>>
>> Having two separate DT binding documents sounds fine to me, if unifying
>> them makes things too complex.
>> However, I think they should use the same compatible value, because the
>> hardware block is the same, but just used in a different mode.
>>
>> Rob/Mark: Any input from the DT maintainers?
> 
> Separate files makes sense because different modes will want to 
> include different common schemas. We've generally been doing different 
> compatibles too which makes validating the node has the right set of 
> properties easier.
>  
>>>>> +- 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.
>>
>> Are they fixed by the PCIe hardware, i.e. could they be looked up by the
>> driver based on the compatible value?
> 
> That would be strange for a memory range.
> 
> Sounds like like 'ranges' though I'm not sure if 'ranges' for an EP 
> makes sense or what that should look like.

These are similar to "memory node" with multiple address, size pairs. I'm
thinking if these should be added as a subnode within PCIe EP controller device
tree node?

Thanks
Kishon

Lad, Prabhakar Nov. 27, 2019, 9 p.m. UTC | #10

Hi Kishon,

On Wed, Nov 27, 2019 at 5:45 AM Kishon Vijay Abraham I <[e-mail]> wrote:
>
> Hi,
>
> On 13/11/19 9:38 AM, Rob Herring wrote:
> > On Thu, Nov 07, 2019 at 09:08:35PM +0100, Geert Uytterhoeven wrote:
> >> Hi Prabhakar,
> >>
> >> On Thu, Nov 7, 2019 at 10:26 AM Lad, Prabhakar
> >> <[e-mail]> wrote:
> >>> On Thu, Nov 7, 2019 at 8:44 AM Geert Uytterhoeven <[e-mail]> wrote:
> >>>> On Wed, Nov 6, 2019 at 8:36 PM Lad Prabhakar <[e-mail]> wrote:
> >>>>> From: "Lad, Prabhakar" <[e-mail]>
> >>>>>
> >>>>> This patch adds the bindings for the R-Car PCIe endpoint driver.
> >>>>>
> >>>>> Signed-off-by: Lad, Prabhakar <[e-mail]>
> >>>>
> >>>> 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             ||
> >>
> >> We have a similar situation with SPI, where a controller can operate in
> >> master or slave mode, based on the absence or presence of the
> >> "spi-slave" DT property.
> >>
> >>> 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
> >>
> >> You can still have two separate drivers, binding against the same
> >> compatible value.  Just let the .probe() function return -ENODEV if it
> >> discovers (by looking at DT properties) if the node is configured for
> >> the other mode.
> >> Which brings us to my next questions: is there any code that could be
> >> shared between the drivers for the two modes?
> >>
> >>> rockchip/cadence/designware where
> >>> its the same hardware block but has two different binding files one
> >>> for host mode and other for
> >>> endpoint mode.
> >>
> >> Having two separate DT binding documents sounds fine to me, if unifying
> >> them makes things too complex.
> >> However, I think they should use the same compatible value, because the
> >> hardware block is the same, but just used in a different mode.
> >>
> >> Rob/Mark: Any input from the DT maintainers?
> >
> > Separate files makes sense because different modes will want to
> > include different common schemas. We've generally been doing different
> > compatibles too which makes validating the node has the right set of
> > properties easier.
> >
> >>>>> +- 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.
> >>
> >> Are they fixed by the PCIe hardware, i.e. could they be looked up by the
> >> driver based on the compatible value?
> >
> > That would be strange for a memory range.
> >
> > Sounds like like 'ranges' though I'm not sure if 'ranges' for an EP
> > makes sense or what that should look like.
>
> These are similar to "memory node" with multiple address, size pairs. I'm
> thinking if these should be added as a subnode within PCIe EP controller device
> tree node?
>
+1

something similar like below ?

       pcie_ep: pcie_ep@fe000000 {
               compatible = "renesas,pcie-r8a7791", "renesas,pcie-rcar-gen2";
               reg = <0 0xfe000000 0 0x80000>;
               reg-names = "apb-base";
               clocks = <&cpg CPG_MOD 319>;
               clock-names = "pcie";
               power-domains = <&sysc R8A774C0_PD_ALWAYS_ON>;
               resets = <&cpg 319>;
               mem-region {
                       base =  <0x0 0xfe100000 0 0x100000>,
                                    <0x0 0xfe200000 0 0x200000>,
                                    <0x0 0x30000000 0 0x8000000>,
                                    <0x0 0x38000000 0 0x8000000>;
                };
       };

Cheers,
--Prabhakar

Patch

diff --git a/Documentation/devicetree/bindings/pci/rcar-pci-ep.txt b/Documentation/devicetree/bindings/pci/rcar-pci-ep.txt
new file mode 100644
index 000000000000..b8c8616ca007
--- /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.
+
+	    When compatible with the generic version, nodes must list the
+	    SoC-specific version corresponding to the platform first
+	    followed by the generic version.
+
+- reg: Five register ranges as listed in the reg-names property
+- reg-names: Must include the following names
+	- "apb-base"
+	- "memory0"
+	- "memory1"
+	- "memory2"
+	- "memory3"
+- 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";
+		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>;
+	};