Re: [PATCH 2/6] dt-bindings: PCI: tegra: Add DT support for PCIe EP nodes in Tegra194
From: Vidya Sagar
Date: Thu Dec 05 2019 - 04:58:09 EST
On 11/29/2019 6:56 PM, Vidya Sagar wrote:
Rob, Can you please update your comments on this?
Thanks,
Vidya Sagar
On 11/25/2019 5:22 PM, Gustavo Pimentel wrote:
On Mon, Nov 25, 2019 at 7:33:59, Thierry Reding
<thierry.reding@xxxxxxxxx> wrote:
On Mon, Nov 25, 2019 at 12:53:42PM +0530, Vidya Sagar wrote:
On 11/22/2019 6:49 PM, Thierry Reding wrote:
On Fri, Nov 22, 2019 at 04:15:01PM +0530, Vidya Sagar wrote:
Add support for PCIe controllers that can operate in endpoint mode
in Tegra194.
Signed-off-by: Vidya Sagar <vidyas@xxxxxxxxxx>
---
 .../bindings/pci/nvidia,tegra194-pcie-ep.txt | 138 ++++++++++++++++++
ÂÂ 1 file changed, 138 insertions(+)
ÂÂ create mode 100644 Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie-ep.txt
The vast majority of this is a duplication of the host mode device tree
bindings. I think it'd be best to combine both and only highlight where
both modes differ.
The designware-pcie.txt binding does something similar.
Ok. I'll merge this into the host mode bindings file and in that differentiate between
root mode and endpoint mode.
diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie-ep.txt b/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie-ep.txt
new file mode 100644
index 000000000000..4676ccf7dfa5
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie-ep.txt
@@ -0,0 +1,138 @@
+NVIDIA Tegra PCIe Endpoint mode controller (Synopsys DesignWare Core based)
+
+Some of the PCIe controllers which are based on Synopsys DesignWare PCIe IP
+are dual mode i.e. they can work in root port mode or endpoint mode but one
+ at a time. Since they are based on DesignWare IP, they inherit all the common
+properties defined in designware-pcie.txt.
+
+Required properties:
+- compatible: For Tegra19x, must contain "nvidia,tegra194-pcie".
The device tree snippets that you added have "nvidia,tegra194-pcie-ep"
for EP mode controllers. So either this is wrong or the DTS files are
wrong.
DTS file are correct. This is a mistake in this file. I'll correct this.
This device tree binding describes the exact same hardware, so I don't
think we necessarily need two different compatible strings. It's fairly
easy to distinguish between which mode to run in by looking at which
properties exist. EP mode for example is the only one that uses the
"addr_space" reg entry.
Rob, do you know why a different compatible string was chosen for the EP
mode? Looking at the driver, there are only a handful of differences in
the programming, but most of the driver remains identical. An extra DT
compatible string seems a bit exaggerated since it suggests that this is
actually different hardware, where it clearly isn't.
Since all other implementations have done it this way, I just followed to be in sync
with them. Even I would also like to hear from Rob on the rationale behind this.
Rob, Could you please update on this?
+Â Tegra194: Only C0, C4 & C5 controllers are dual mode controllers.
+- power-domains: A phandle to the node that controls power to the respective
+Â PCIe controller and a specifier name for the PCIe controller. Following are
+Â the specifiers for the different PCIe controllers
+ÂÂÂ TEGRA194_POWER_DOMAIN_PCIEX8B: C0
+ÂÂÂ TEGRA194_POWER_DOMAIN_PCIEX4A: C4
+ÂÂÂ TEGRA194_POWER_DOMAIN_PCIEX8A: C5
+Â these specifiers are defined in
+Â "include/dt-bindings/power/tegra194-powergate.h" file.
+- reg: A list of physical base address and length pairs for each set of
+Â controller registers. Must contain an entry for each entry in the reg-names
+Â property.
+- reg-names: Must include the following entries:
+Â "appl": Controller's application logic registers
+Â "atu_dma": iATU and DMA registers. This is where the iATU (internal Address
+ÂÂÂÂÂÂÂÂÂÂÂÂ Translation Unit) registers of the PCIe core are made available
+ÂÂÂÂÂÂÂÂÂÂÂÂ for SW access.
+Â "dbi": The aperture where root port's own configuration registers are
+ÂÂÂÂÂÂÂÂ available
+Â "addr_space": Used to map remote RC address space
+- interrupts: A list of interrupt outputs of the controller. Must contain an
+Â entry for each entry in the interrupt-names property.
+- interrupt-names: Must include the following entry:
+Â "intr": The Tegra interrupt that is asserted for controller interrupts
+- clocks: Must contain an entry for each entry in clock-names.
+Â See ../clocks/clock-bindings.txt for details.
+- clock-names: Must include the following entries:
+Â - core
+- resets: Must contain an entry for each entry in reset-names.
+Â See ../reset/reset.txt for details.
+- reset-names: Must include the following entries:
+Â - apb
+Â - core
+- phys: Must contain a phandle to P2U PHY for each entry in phy-names.
+- phy-names: Must include an entry for each active lane.
+Â "p2u-N": where N ranges from 0 to one less than the total number of lanes
+- nvidia,bpmp: Must contain a pair of phandle to BPMP controller node followed
+Â by controller-id. Following are the controller ids for each controller.
+ÂÂÂ 0: C0
+ÂÂÂ 4: C4
+ÂÂÂ 5: C5
+- vddio-pex-ctl-supply: Regulator supply for PCIe side band signals
+- nvidia,pex-rst-gpio: Must contain a phandle to a GPIO controller followed by
+Â GPIO that is being used as PERST signal
Why is this NVIDIA specific? Do other instantiations of the DW IP not
also need a means to define which GPIO is the reset?
I'm not sure. At least I didn't find anything like this in other implementations.
My understanding is that, controller handles assert/de-assert on the PERST line
automatically without SW intervention. I think it is for the same reason that other
implementations don't wait for the REFCLK to flow in from host to configure the IP.
I think they just use some internal clock for the configuration and switch to
running the core based on REFCLK as and when it is available
(i.e. whenever a de-assert is perceived on PERST line by the controller)
That would be somewhat surprising, though. The IP used in Tegra must be
pretty close to the IP used in other SoCs, and the code that we need in
pex_ep_event_pex_rst_{assert,deassert}() is pretty significant. Why the
other instantiations wouldn't need something similar seems unlikely to
me.
Perhaps Jingoo or Gustavo can shed some light on this.
On my current FPGA prototyping solution, I don't need to control the
PERST line and it's very likely that I don't even have access to control
it. I guess due to some particularity of my solution, the HW team
probably has decided to wire it up directly for some unknown reason to
me.
However, It seems to me that exynos, imx6, keystone, meson, al, histb,
kirin, and qcom drivers controls the PERST line in spite of others driver
that doesn't do it like in my prototype solution.
In the end I'd says that depends of how the IP solution of design by the
HW team.
Gustavo
Thierry
+
+Optional properties:
+- pinctrl-names: A list of pinctrl state names.
+Â It is mandatory for C5 controller and optional for other controllers.
+Â - "default": Configures PCIe I/O for proper operation.
+- pinctrl-0: phandle for the 'default' state of pin configuration.
+Â It is mandatory for C5 controller and optional for other controllers.
+- supports-clkreq: Refer to Documentation/devicetree/bindings/pci/pci.txt
+- nvidia,update-fc-fixup: This is a boolean property and needs to be present to
+ÂÂÂ improve performance when a platform is designed in such a way that it
+ÂÂÂ satisfies at least one of the following conditions thereby enabling root
+ÂÂÂ port to exchange optimum number of FC (Flow Control) credits with
+ÂÂÂ downstream devices
+ÂÂÂ 1. If C0/C4/C5 run at x1/x2 link widths (irrespective of speed and MPS)
+ÂÂÂ 2. If C0/C4/C5 operate at their respective max link widths and
+ÂÂÂÂÂÂ a) speed is Gen-2 and MPS is 256B
+ÂÂÂÂÂÂ b) speed is >= Gen-3 with any MPS
+- nvidia,aspm-cmrt-us: Common Mode Restore Time for proper operation of ASPM
+ÂÂ to be specified in microseconds
+- nvidia,aspm-pwr-on-t-us: Power On time for proper operation of ASPM to be
+ÂÂ specified in microseconds
+- nvidia,aspm-l0s-entrance-latency-us: ASPM L0s entrance latency to be
+ÂÂ specified in microseconds
+
+NOTE:- On Tegra194's P2972-0000 platform, only C5 controller can be enabled to
+operate in the endpoint mode because of the way the platform is designed.
+There is a mux that needs to be programmed to let the REFCLK from the host to
+flow into C5 controller when it operates in the endpoint mode. This mux is
+controlled by the GPIO (AA, 5) and it needs to be driven 'high'. For this to
+happen, set status of "pex-refclk-sel-high" node under "gpio@c2f0000" node to
+'okay'.
+ÂÂÂ When any dual mode controller is made to operate in the endpoint mode,
+please make sure that its respective root port node's status is set to
+'disabled'.
This seems very brittle to me. There's no good way how we can detect
such misconfigurations. If instead we only have one node describing the
hardware fully, the chances of configuring things wrong (by for example
enabling both the host and EP mode device tree nodes) can be reduced.
So I think instead of duplicating all of the device tree content to have
both a host and an EP node for each controller, it'd be better to just
have a single node and let the device tree bindings specify which
changes to apply to switch into EP mode.
For example, there should be nothing wrong with specifying some of the
EP-only properties (like num-ib-windows and num-ob-windows) all the time
and only use them when we actually run in EP mode.
As I mentioned earlier, there are a couple of easy ways to distinguish
the modes. The presence of the "addr_space" reg entry is one example,
but we could also key off the nvidia,pex-rst-gpio property, since that
(presumably) wouldn't be needed for host mode.
That way we can just add default, host mode entries to tegra194.dtsi and
whenever somebody wants to enable EP mode, they can just override the
node in the board-level DTS file, like so:
ÂÂÂÂpcie@141a0000 {
ÂÂÂÂÂÂÂ reg = <0x00 0x141a0000 0x0 0x00020000ÂÂ /* appl registers (128K)ÂÂÂÂÂ */
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x00 0x3a040000 0x0 0x00040000ÂÂ /* iATU_DMA reg space (256K)Â */
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x00 0x3a080000 0x0 0x00040000ÂÂ /* DBI reg space (256K)ÂÂÂÂÂÂ */
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x1c 0x00000000 0x4 0x00000000>; /* Address Space (16G)ÂÂÂÂÂÂÂ */
ÂÂÂÂÂÂÂ reg-names = "appl", "atu_dma", "dbi", "addr_space";
ÂÂÂÂÂÂÂ nvidia,pex-rst-gpio = <&gpio TEGRA194_MAIN_GPIO(GG, 1) GPIO_ACTIVE_LOW>;
ÂÂÂÂ};
Thierry
I like it and fine with making these modifications also but would like to hear from Rob
also on this.
- Vidya Sagar
+
+Examples:
+=========
+
+Tegra194:
+--------
+
+ÂÂÂ pcie_ep@141a0000 {
+ÂÂÂÂÂÂÂ compatible = "nvidia,tegra194-pcie-ep", "snps,dw-pcie-ep";
+ÂÂÂÂÂÂÂ power-domains = <&bpmp TEGRA194_POWER_DOMAIN_PCIEX8A>;
+ÂÂÂÂÂÂÂ reg = <0x00 0x141a0000 0x0 0x00020000ÂÂ /* appl registers (128K)ÂÂÂÂÂ */
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x00 0x3a040000 0x0 0x00040000ÂÂ /* iATU_DMA reg space (256K)Â */
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x00 0x3a080000 0x0 0x00040000ÂÂ /* DBI reg space (256K)ÂÂÂÂÂÂ */
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x1c 0x00000000 0x4 0x00000000>; /* Address Space (16G)ÂÂÂÂÂÂÂ */
+ÂÂÂÂÂÂÂ reg-names = "appl", "atu_dma", "dbi", "addr_space";
+
+ÂÂÂÂÂÂÂ num-lanes = <8>;
+ÂÂÂÂÂÂÂ num-ib-windows = <2>;
+ÂÂÂÂÂÂÂ num-ob-windows = <8>;
+
+ÂÂÂÂÂÂÂ pinctrl-names = "default";
+ÂÂÂÂÂÂÂ pinctrl-0 = <&clkreq_c5_bi_dir_state>;
+
+ÂÂÂÂÂÂÂ clocks = <&bpmp TEGRA194_CLK_PEX1_CORE_5>;
+ÂÂÂÂÂÂÂ clock-names = "core";
+
+ÂÂÂÂÂÂÂ resets = <&bpmp TEGRA194_RESET_PEX1_CORE_5_APB>,
+ÂÂÂÂÂÂÂÂÂÂÂÂ <&bpmp TEGRA194_RESET_PEX1_CORE_5>;
+ÂÂÂÂÂÂÂ reset-names = "apb", "core";
+
+ÂÂÂÂÂÂÂ interrupts = <GIC_SPI 53 IRQ_TYPE_LEVEL_HIGH>;ÂÂÂ /* controller interrupt */
+ÂÂÂÂÂÂÂ interrupt-names = "intr";
+
+ÂÂÂÂÂÂÂ nvidia,bpmp = <&bpmp 5>;
+
+ÂÂÂÂÂÂÂ nvidia,aspm-cmrt-us = <60>;
+ÂÂÂÂÂÂÂ nvidia,aspm-pwr-on-t-us = <20>;
+ÂÂÂÂÂÂÂ nvidia,aspm-l0s-entrance-latency-us = <3>;
+
+ÂÂÂÂÂÂÂ vddio-pex-ctl-supply = <&vdd_1v8ao>;
+
+ÂÂÂÂÂÂÂ nvidia,pex-rst-gpio = <&gpio TEGRA194_MAIN_GPIO(GG, 1)
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ GPIO_ACTIVE_LOW>;
+
+ÂÂÂÂÂÂÂ phys = <&p2u_nvhs_0>, <&p2u_nvhs_1>, <&p2u_nvhs_2>,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ <&p2u_nvhs_3>, <&p2u_nvhs_4>, <&p2u_nvhs_5>,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ <&p2u_nvhs_6>, <&p2u_nvhs_7>;
+
+ÂÂÂÂÂÂÂ phy-names = "p2u-0", "p2u-1", "p2u-2", "p2u-3", "p2u-4",
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "p2u-5", "p2u-6", "p2u-7";
+ÂÂÂ };
--
2.17.1