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? Thanks Kishon
