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

[Lad, Prabhakar]

Hi Kishon,

On Wed, Nov 27, 2019 at 5:45 AM Kishon Vijay Abraham I <kishon@xxxxxx> 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
> >> <prabhakar.csengg@xxxxxxxxx> wrote:
> >>> On Thu, Nov 7, 2019 at 8:44 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> >>>> 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             ||
> >>
> >> 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