[PATCH 0/6] Add support for PCIe endpoint mode in Tegra194

[PATCH 0/6] Add support for PCIe endpoint mode in Tegra194

[Vidya Sagar]

Tegra194 has three (C0, C4 & C5) dual mode PCIe controllers that can operate
either in root port mode or in end point mode but only in one mode at a time.
Platform P2972-0000 supports enabling endpoint mode for C5 controller. This
patch series adds support for PCIe endpoint mode in both the driver as well as
in DT.
This patch series depends on the changes made for Synopsys DesignWare endpoint
mode subsystem that are currently under review
@ https://patchwork.kernel.org/project/linux-pci/list/?series=202211
which in turn depends on the patch made by Kishon
@ https://patchwork.kernel.org/patch/10975123/
which is also under review.

Vidya Sagar (6):
  soc/tegra: bpmp: Update ABI header
  dt-bindings: PCI: tegra: Add DT support for PCIe EP nodes in Tegra194
  PCI: tegra: Add support for PCIe endpoint mode in Tegra194
  arm64: tegra: Add PCIe endpoint controllers nodes for Tegra194
  arm64: tegra: Enable GPIO controllers nodes for P2972-0000 platform
  arm64: tegra: Add support for PCIe endpoint mode in P2972-0000
    platform

 .../bindings/pci/nvidia,tegra194-pcie-ep.txt  | 138 ++++
 .../boot/dts/nvidia/tegra194-p2972-0000.dts   |  37 +
 arch/arm64/boot/dts/nvidia/tegra194.dtsi      |  99 +++
 drivers/pci/controller/dwc/Kconfig            |  30 +-
 drivers/pci/controller/dwc/pcie-tegra194.c    | 751 +++++++++++++++++-
 include/soc/tegra/bpmp-abi.h                  |  10 +-
 6 files changed, 1048 insertions(+), 17 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie-ep.txt

-- 
2.17.1

[PATCH 1/6] soc/tegra: bpmp: Update ABI header

[Vidya Sagar]

Update the firmware header to support uninitialization of UPHY PLL
when the PCIe controller is operating in endpoint mode and host cuts
the PCIe reference clock.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
 include/soc/tegra/bpmp-abi.h | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/soc/tegra/bpmp-abi.h b/include/soc/tegra/bpmp-abi.h
index cac6f610b3fe..322d06e21918 100644
--- a/include/soc/tegra/bpmp-abi.h
+++ b/include/soc/tegra/bpmp-abi.h
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 /*
- * Copyright (c) 2014-2018, NVIDIA CORPORATION.  All rights reserved.
+ * Copyright (c) 2014-2019, NVIDIA CORPORATION.  All rights reserved.
  */
 
 #ifndef _ABI_BPMP_ABI_H_
@@ -2119,6 +2119,7 @@ enum {
 	CMD_UPHY_PCIE_LANE_MARGIN_STATUS = 2,
 	CMD_UPHY_PCIE_EP_CONTROLLER_PLL_INIT = 3,
 	CMD_UPHY_PCIE_CONTROLLER_STATE = 4,
+	CMD_UPHY_PCIE_EP_CONTROLLER_PLL_OFF = 5,
 	CMD_UPHY_MAX,
 };
 
@@ -2151,6 +2152,11 @@ struct cmd_uphy_pcie_controller_state_request {
 	uint8_t enable;
 } __ABI_PACKED;
 
+struct cmd_uphy_ep_controller_pll_off_request {
+	/** @brief EP controller number, valid: 0, 4, 5 */
+	uint8_t ep_controller;
+} __ABI_PACKED;
+
 /**
  * @ingroup UPHY
  * @brief Request with #MRQ_UPHY
@@ -2165,6 +2171,7 @@ struct cmd_uphy_pcie_controller_state_request {
  * |CMD_UPHY_PCIE_LANE_MARGIN_STATUS     |                                        |
  * |CMD_UPHY_PCIE_EP_CONTROLLER_PLL_INIT |cmd_uphy_ep_controller_pll_init_request |
  * |CMD_UPHY_PCIE_CONTROLLER_STATE       |cmd_uphy_pcie_controller_state_request  |
+ * |CMD_UPHY_PCIE_EP_CONTROLLER_PLL_OFF  |cmd_uphy_ep_controller_pll_off_request  |
  *
  */
 
@@ -2178,6 +2185,7 @@ struct mrq_uphy_request {
 		struct cmd_uphy_margin_control_request uphy_set_margin_control;
 		struct cmd_uphy_ep_controller_pll_init_request ep_ctrlr_pll_init;
 		struct cmd_uphy_pcie_controller_state_request controller_state;
+		struct cmd_uphy_ep_controller_pll_off_request ep_ctrlr_pll_off;
 	} __UNION_ANON;
 } __ABI_PACKED;
 
-- 
2.17.1

[PATCH 2/6] dt-bindings: PCI: tegra: Add DT support for PCIe EP nodes in Tegra194

[Vidya Sagar]

Add support for PCIe controllers that can operate in endpoint mode
in Tegra194.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
 .../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

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

Re: [PATCH 2/6] dt-bindings: PCI: tegra: Add DT support for PCIe EP nodes in Tegra194

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 10217 bytes --]

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@nvidia.com>
> ---
>  .../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.

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

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.

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

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/6] dt-bindings: PCI: tegra: Add DT support for PCIe EP nodes in Tegra194

[Vidya Sagar]

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@nvidia.com>
>> ---
>>   .../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.

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

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

Re: [PATCH 2/6] dt-bindings: PCI: tegra: Add DT support for PCIe EP nodes in Tegra194

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 12661 bytes --]

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@nvidia.com>
> > > ---
> > >   .../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.
> 
> > 
> > > +  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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 2/6] dt-bindings: PCI: tegra: Add DT support for PCIe EP nodes in Tegra194

[Gustavo Pimentel]

On Mon, Nov 25, 2019 at 7:33:59, Thierry Reding 
<thierry.reding@gmail.com> 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@nvidia.com>
> > > > ---
> > > >   .../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.
> > 
> > > 
> > > > +  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
> > > > 
> > 

Re: [PATCH 2/6] dt-bindings: PCI: tegra: Add DT support for PCIe EP nodes in Tegra194

[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@gmail.com> 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@nvidia.com>
>>>>> ---
>>>>>    .../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
>>>>>
>>>
> 
> 

Re: [PATCH 2/6] dt-bindings: PCI: tegra: Add DT support for PCIe EP nodes in Tegra194

[Vidya Sagar]

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@gmail.com> 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@nvidia.com>
>>>>>> ---
>>>>>>    .../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
>>>>>>
>>>>
>>
>>
> 

Re: [PATCH 2/6] dt-bindings: PCI: tegra: Add DT support for PCIe EP nodes in Tegra194

[Rob Herring]

On Fri, Nov 22, 2019 at 02:19:31PM +0100, 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@nvidia.com>
> > ---
> >  .../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.

That will not work so well as a schema because all the common PCI bus 
related properties don't apply. Child nodes if any for an EP aren't PCI 
devices either. Though you could have 3 files with common properties, 
host binding and ep bindings. 

While we don't have anything in terms of common PCI EP bindings, I 
somewhat expect that to change. There's been something for how to define 
multiple EP functions for example.

> The designware-pcie.txt binding does something similar.
> 
> > 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.
> 
> 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.

Whether the driver shares a lot of code or not, it's fundamentally a 
different device and driver stack.

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

'reset-gpios' is used for PERST. Though in this case it's an input 
rather than output.

Rob

[PATCH 3/6] PCI: tegra: Add support for PCIe endpoint mode in Tegra194

[Vidya Sagar]

Add support for the endpoint mode of Synopsys DesignWare core based
dual mode PCIe controllers present in Tegra194 SoC.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
 drivers/pci/controller/dwc/Kconfig         |  30 +-
 drivers/pci/controller/dwc/pcie-tegra194.c | 751 ++++++++++++++++++++-
 2 files changed, 765 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 0ba988b5b5bc..32165a77c94c 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -237,14 +237,38 @@ config PCI_MESON
 	  implement the driver.
 
 config PCIE_TEGRA194
-	tristate "NVIDIA Tegra194 (and later) PCIe controller"
+	tristate
+
+config PCIE_TEGRA194_HOST
+	tristate "NVIDIA Tegra194 (and later) PCIe controller - Host Mode"
 	depends on ARCH_TEGRA_194_SOC || COMPILE_TEST
 	depends on PCI_MSI_IRQ_DOMAIN
 	select PCIE_DW_HOST
 	select PHY_TEGRA194_P2U
+	select PCIE_TEGRA194
+	default y
+	help
+	  Enables support for the PCIe controller in the NVIDIA Tegra194 SoC to
+	  work in host mode. There are two instances of PCIe controllers in
+	  Tegra194. This controller can work either as EP or RC. In order to
+	  enable host-specific features PCIE_TEGRA194_HOST must be selected and
+	  in order to enable device-specific features PCIE_TEGRA194_EP must be
+	  selected. This uses the DesignWare core.
+
+config PCIE_TEGRA194_EP
+	tristate "NVIDIA Tegra194 (and later) PCIe controller - Endpoint Mode"
+	depends on ARCH_TEGRA_194_SOC || COMPILE_TEST
+	depends on PCI_ENDPOINT
+	select PCIE_DW_EP
+	select PHY_TEGRA194_P2U
+	select PCIE_TEGRA194
 	help
-	  Say Y here if you want support for DesignWare core based PCIe host
-	  controller found in NVIDIA Tegra194 SoC.
+	  Enables support for the PCIe controller in the NVIDIA Tegra194 SoC to
+	  work in host mode. There are two instances of PCIe controllers in
+	  Tegra194. This controller can work either as EP or RC. In order to
+	  enable host-specific features PCIE_TEGRA194_HOST must be selected and
+	  in order to enable device-specific features PCIE_TEGRA194_EP must be
+	  selected. This uses the DesignWare core.
 
 config PCIE_UNIPHIER
 	bool "Socionext UniPhier PCIe controllers"
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index cbe95f0ea0ca..f533cba51c2f 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -14,6 +14,8 @@
 #include <linux/interrupt.h>
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
+#include <linux/kfifo.h>
+#include <linux/kthread.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -53,6 +55,7 @@
 #define APPL_INTR_EN_L0_0_LINK_STATE_INT_EN	BIT(0)
 #define APPL_INTR_EN_L0_0_MSI_RCV_INT_EN	BIT(4)
 #define APPL_INTR_EN_L0_0_INT_INT_EN		BIT(8)
+#define APPL_INTR_EN_L0_0_PCI_CMD_EN_INT_EN	BIT(15)
 #define APPL_INTR_EN_L0_0_CDM_REG_CHK_INT_EN	BIT(19)
 #define APPL_INTR_EN_L0_0_SYS_INTR_EN		BIT(30)
 #define APPL_INTR_EN_L0_0_SYS_MSI_INTR_EN	BIT(31)
@@ -60,19 +63,26 @@
 #define APPL_INTR_STATUS_L0			0xC
 #define APPL_INTR_STATUS_L0_LINK_STATE_INT	BIT(0)
 #define APPL_INTR_STATUS_L0_INT_INT		BIT(8)
+#define APPL_INTR_STATUS_L0_PCI_CMD_EN_INT	BIT(15)
+#define APPL_INTR_STATUS_L0_PEX_RST_INT		BIT(16)
 #define APPL_INTR_STATUS_L0_CDM_REG_CHK_INT	BIT(18)
 
 #define APPL_INTR_EN_L1_0_0				0x1C
 #define APPL_INTR_EN_L1_0_0_LINK_REQ_RST_NOT_INT_EN	BIT(1)
+#define APPL_INTR_EN_L1_0_0_RDLH_LINK_UP_INT_EN		BIT(3)
+#define APPL_INTR_EN_L1_0_0_HOT_RESET_DONE_INT_EN	BIT(30)
 
 #define APPL_INTR_STATUS_L1_0_0				0x20
 #define APPL_INTR_STATUS_L1_0_0_LINK_REQ_RST_NOT_CHGED	BIT(1)
+#define APPL_INTR_STATUS_L1_0_0_RDLH_LINK_UP_CHGED	BIT(3)
+#define APPL_INTR_STATUS_L1_0_0_HOT_RESET_DONE		BIT(30)
 
 #define APPL_INTR_STATUS_L1_1			0x2C
 #define APPL_INTR_STATUS_L1_2			0x30
 #define APPL_INTR_STATUS_L1_3			0x34
 #define APPL_INTR_STATUS_L1_6			0x3C
 #define APPL_INTR_STATUS_L1_7			0x40
+#define APPL_INTR_STATUS_L1_15_CFG_BME_CHGED	BIT(1)
 
 #define APPL_INTR_EN_L1_8_0			0x44
 #define APPL_INTR_EN_L1_8_BW_MGT_INT_EN		BIT(2)
@@ -103,8 +113,12 @@
 #define APPL_INTR_STATUS_L1_18_CDM_REG_CHK_CMP_ERR	BIT(1)
 #define APPL_INTR_STATUS_L1_18_CDM_REG_CHK_LOGIC_ERR	BIT(0)
 
+#define APPL_MSI_CTRL_1				0xAC
+
 #define APPL_MSI_CTRL_2				0xB0
 
+#define APPL_LEGACY_INTX			0xB8
+
 #define APPL_LTR_MSG_1				0xC4
 #define LTR_MSG_REQ				BIT(15)
 #define LTR_MST_NO_SNOOP_SHIFT			16
@@ -205,6 +219,13 @@
 #define AMBA_ERROR_RESPONSE_CRS_OKAY_FFFFFFFF	1
 #define AMBA_ERROR_RESPONSE_CRS_OKAY_FFFF0001	2
 
+#define MSIX_ADDR_MATCH_LOW_OFF			0x940
+#define MSIX_ADDR_MATCH_LOW_OFF_EN		BIT(0)
+#define MSIX_ADDR_MATCH_LOW_OFF_MASK		GENMASK(31, 2)
+
+#define MSIX_ADDR_MATCH_HIGH_OFF		0x944
+#define MSIX_ADDR_MATCH_HIGH_OFF_MASK		GENMASK(31, 0)
+
 #define PORT_LOGIC_MSIX_DOORBELL			0x948
 
 #define CAP_SPCIE_CAP_OFF			0x154
@@ -223,6 +244,25 @@
 #define GEN3_CORE_CLK_FREQ	250000000
 #define GEN4_CORE_CLK_FREQ	500000000
 
+#define LTR_MSG_TIMEOUT		(100 * 1000)
+
+#define PERST_DEBOUNCE_TIME	(5 * 1000)
+
+#define EVENT_QUEUE_LEN		(256)
+
+#define EP_STATE_DISABLED	0
+#define EP_STATE_ENABLED	1
+
+enum ep_event {
+	EP_EVENT_NONE = 0,
+	EP_PEX_RST_DEASSERT,
+	EP_PEX_RST_ASSERT,
+	EP_HOT_RST_DONE,
+	EP_BME_CHANGE,
+	EP_EVENT_EXIT,
+	EP_EVENT_INVALID,
+};
+
 static const unsigned int pcie_gen_freq[] = {
 	GEN1_CORE_CLK_FREQ,
 	GEN2_CORE_CLK_FREQ,
@@ -260,6 +300,8 @@ struct tegra_pcie_dw {
 	struct dw_pcie pci;
 	struct tegra_bpmp *bpmp;
 
+	enum dw_pcie_device_mode mode;
+
 	bool supports_clkreq;
 	bool enable_cdm_check;
 	bool link_state;
@@ -283,6 +325,18 @@ struct tegra_pcie_dw {
 	struct phy **phys;
 
 	struct dentry *debugfs;
+
+	/* Endpoint mode specific */
+	struct task_struct *pcie_ep_task;
+	wait_queue_head_t wq;
+	struct gpio_desc *pex_rst_gpiod;
+	unsigned int pex_rst_irq;
+	int ep_state;
+	DECLARE_KFIFO(event_fifo, u32, EVENT_QUEUE_LEN);
+};
+
+struct tegra_pcie_dw_of_data {
+	enum dw_pcie_device_mode mode;
 };
 
 static inline struct tegra_pcie_dw *to_tegra_pcie(struct dw_pcie *pci)
@@ -411,11 +465,59 @@ static irqreturn_t tegra_pcie_rp_irq_handler(struct tegra_pcie_dw *pcie)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t tegra_pcie_ep_irq_handler(struct tegra_pcie_dw *pcie)
+{
+	struct dw_pcie_ep *ep = &pcie->pci.ep;
+	u32 val, tmp;
+
+	val = appl_readl(pcie, APPL_INTR_STATUS_L0);
+	if (val & APPL_INTR_STATUS_L0_LINK_STATE_INT) {
+		val = appl_readl(pcie, APPL_INTR_STATUS_L1_0_0);
+		appl_writel(pcie, val, APPL_INTR_STATUS_L1_0_0);
+		if (val & APPL_INTR_STATUS_L1_0_0_HOT_RESET_DONE) {
+			/* clear any stale PEX_RST interrupt */
+			if (!kfifo_put(&pcie->event_fifo, EP_HOT_RST_DONE)) {
+				dev_err(pcie->dev, "EVENT FIFO is full\n");
+				return IRQ_HANDLED;
+			}
+			wake_up(&pcie->wq);
+		}
+		if (val & APPL_INTR_STATUS_L1_0_0_RDLH_LINK_UP_CHGED) {
+			tmp = appl_readl(pcie, APPL_LINK_STATUS);
+			if (tmp & APPL_LINK_STATUS_RDLH_LINK_UP) {
+				dev_info(pcie->dev, "Link is up with Host\n");
+				dw_pcie_ep_linkup(ep);
+			}
+		}
+	} else if (val & APPL_INTR_STATUS_L0_PCI_CMD_EN_INT) {
+		val = appl_readl(pcie, APPL_INTR_STATUS_L1_15);
+		appl_writel(pcie, val, APPL_INTR_STATUS_L1_15);
+		if (val & APPL_INTR_STATUS_L1_15_CFG_BME_CHGED) {
+			if (!kfifo_put(&pcie->event_fifo, EP_BME_CHANGE)) {
+				dev_err(pcie->dev, "EVENT FIFO is full\n");
+				return IRQ_HANDLED;
+			}
+			wake_up(&pcie->wq);
+		}
+	} else {
+		dev_warn(pcie->dev, "Random interrupt (STATUS = 0x%08X)\n",
+			 val);
+		appl_writel(pcie, val, APPL_INTR_STATUS_L0);
+	}
+
+	return IRQ_HANDLED;
+}
+
 static irqreturn_t tegra_pcie_irq_handler(int irq, void *arg)
 {
 	struct tegra_pcie_dw *pcie = arg;
 
-	return tegra_pcie_rp_irq_handler(pcie);
+	if (pcie->mode == DW_PCIE_RC_TYPE)
+		return tegra_pcie_rp_irq_handler(pcie);
+	else if (pcie->mode == DW_PCIE_EP_TYPE)
+		return tegra_pcie_ep_irq_handler(pcie);
+
+	return IRQ_NONE;
 }
 
 static int tegra_pcie_dw_rd_own_conf(struct pcie_port *pp, int where, int size,
@@ -884,8 +986,26 @@ static void tegra_pcie_set_msi_vec_num(struct pcie_port *pp)
 	pp->num_vectors = MAX_MSI_IRQS;
 }
 
+static int tegra_pcie_dw_start_link(struct dw_pcie *pci)
+{
+	struct tegra_pcie_dw *pcie = to_tegra_pcie(pci);
+
+	enable_irq(pcie->pex_rst_irq);
+
+	return 0;
+}
+
+static void tegra_pcie_dw_stop_link(struct dw_pcie *pci)
+{
+	struct tegra_pcie_dw *pcie = to_tegra_pcie(pci);
+
+	disable_irq(pcie->pex_rst_irq);
+}
+
 static const struct dw_pcie_ops tegra_dw_pcie_ops = {
 	.link_up = tegra_pcie_dw_link_up,
+	.start_link = tegra_pcie_dw_start_link,
+	.stop_link = tegra_pcie_dw_stop_link,
 };
 
 static struct dw_pcie_host_ops tegra_pcie_dw_host_ops = {
@@ -935,6 +1055,7 @@ static int tegra_pcie_enable_phy(struct tegra_pcie_dw *pcie)
 static int tegra_pcie_dw_parse_dt(struct tegra_pcie_dw *pcie)
 {
 	struct device_node *np = pcie->dev->of_node;
+	char *name;
 	int ret;
 
 	ret = of_property_read_u32(np, "nvidia,aspm-cmrt-us", &pcie->aspm_cmrt);
@@ -986,6 +1107,25 @@ static int tegra_pcie_dw_parse_dt(struct tegra_pcie_dw *pcie)
 	pcie->enable_cdm_check =
 		of_property_read_bool(np, "snps,enable-cdm-check");
 
+	if (pcie->mode == DW_PCIE_RC_TYPE)
+		return 0;
+
+	/* Endpoint mode specific DT entries */
+	name = devm_kasprintf(pcie->dev, GFP_KERNEL,
+			      "tegra_pcie_%u_pex_rst_gpio", pcie->cid);
+	if (!name) {
+		dev_err(pcie->dev, "Failed to create PERST GPIO string\n");
+		return -ENOMEM;
+	}
+	pcie->pex_rst_gpiod = devm_gpiod_get_from_of_node(pcie->dev, np,
+							  "nvidia,pex-rst-gpio",
+							  0, GPIOD_IN, name);
+	if (IS_ERR(pcie->pex_rst_gpiod)) {
+		dev_err(pcie->dev, "Failed to get PERST GPIO: %ld\n",
+			PTR_ERR(pcie->pex_rst_gpiod));
+		return PTR_ERR(pcie->pex_rst_gpiod);
+	}
+
 	return 0;
 }
 
@@ -1017,6 +1157,34 @@ static int tegra_pcie_bpmp_set_ctrl_state(struct tegra_pcie_dw *pcie,
 	return tegra_bpmp_transfer(pcie->bpmp, &msg);
 }
 
+static int tegra_pcie_bpmp_set_pll_state(struct tegra_pcie_dw *pcie,
+					 bool enable)
+{
+	struct mrq_uphy_response resp;
+	struct tegra_bpmp_message msg;
+	struct mrq_uphy_request req;
+
+	memset(&req, 0, sizeof(req));
+	memset(&resp, 0, sizeof(resp));
+
+	if (enable) {
+		req.cmd = CMD_UPHY_PCIE_EP_CONTROLLER_PLL_INIT;
+		req.ep_ctrlr_pll_init.ep_controller = pcie->cid;
+	} else {
+		req.cmd = CMD_UPHY_PCIE_EP_CONTROLLER_PLL_OFF;
+		req.ep_ctrlr_pll_off.ep_controller = pcie->cid;
+	}
+
+	memset(&msg, 0, sizeof(msg));
+	msg.mrq = MRQ_UPHY;
+	msg.tx.data = &req;
+	msg.tx.size = sizeof(req);
+	msg.rx.data = &resp;
+	msg.rx.size = sizeof(resp);
+
+	return tegra_bpmp_transfer(pcie->bpmp, &msg);
+}
+
 static void tegra_pcie_downstream_dev_to_D0(struct tegra_pcie_dw *pcie)
 {
 	struct pcie_port *pp = &pcie->pci.pp;
@@ -1427,8 +1595,552 @@ static int tegra_pcie_config_rp(struct tegra_pcie_dw *pcie)
 	return ret;
 }
 
+static void pex_ep_event_pex_rst_assert(struct tegra_pcie_dw *pcie)
+{
+	u32 val;
+	int ret;
+
+	if (pcie->ep_state == EP_STATE_DISABLED)
+		return;
+
+	/* Disable LTSSM */
+	val = appl_readl(pcie, APPL_CTRL);
+	val &= ~APPL_CTRL_LTSSM_EN;
+	appl_writel(pcie, val, APPL_CTRL);
+
+	ret = readl_poll_timeout(pcie->appl_base + APPL_DEBUG, val,
+				 ((val & APPL_DEBUG_LTSSM_STATE_MASK) >>
+				 APPL_DEBUG_LTSSM_STATE_SHIFT) ==
+				 LTSSM_STATE_PRE_DETECT,
+				 1, LTSSM_TIMEOUT);
+	if (ret)
+		dev_err(pcie->dev, "Failed to go Detect state: %d\n", ret);
+
+	reset_control_assert(pcie->core_rst);
+
+	tegra_pcie_disable_phy(pcie);
+
+	reset_control_assert(pcie->core_apb_rst);
+
+	clk_disable_unprepare(pcie->core_clk);
+
+	pm_runtime_put_sync(pcie->dev);
+
+	ret = tegra_pcie_bpmp_set_pll_state(pcie, false);
+	if (ret)
+		dev_err(pcie->dev, "Failed to turn off UPHY: %d\n", ret);
+
+	pcie->ep_state = EP_STATE_DISABLED;
+	dev_dbg(pcie->dev, "Uninitialization of endpoint is completed\n");
+}
+
+static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie)
+{
+	struct dw_pcie *pci = &pcie->pci;
+	struct dw_pcie_ep *ep = &pci->ep;
+	struct device *dev = pcie->dev;
+	u32 val;
+	int ret;
+
+	if (pcie->ep_state == EP_STATE_ENABLED)
+		return;
+
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		dev_err(dev, "Failed to get runtime sync for PCIe dev: %d\n",
+			ret);
+		return;
+	}
+
+	ret = tegra_pcie_bpmp_set_pll_state(pcie, true);
+	if (ret) {
+		dev_err(dev, "Failed to init UPHY for PCIe EP: %d\n", ret);
+		goto fail_pll_init;
+	}
+
+	ret = clk_prepare_enable(pcie->core_clk);
+	if (ret) {
+		dev_err(dev, "Failed to enable core clock: %d\n", ret);
+		goto fail_core_clk_enable;
+	}
+
+	ret = reset_control_deassert(pcie->core_apb_rst);
+	if (ret) {
+		dev_err(dev, "Failed to deassert core APB reset: %d\n", ret);
+		goto fail_core_apb_rst;
+	}
+
+	ret = tegra_pcie_enable_phy(pcie);
+	if (ret) {
+		dev_err(dev, "Failed to enable PHY: %d\n", ret);
+		goto fail_phy;
+	}
+
+	/* Clear any stale interrupt statuses */
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L0);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_0_0);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_1);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_2);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_3);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_6);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_7);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_8_0);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_9);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_10);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_11);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_13);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_14);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_15);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_17);
+
+	/* configure this core for EP mode operation */
+	val = appl_readl(pcie, APPL_DM_TYPE);
+	val &= ~APPL_DM_TYPE_MASK;
+	val |= APPL_DM_TYPE_EP;
+	appl_writel(pcie, val, APPL_DM_TYPE);
+
+	appl_writel(pcie, 0x0, APPL_CFG_SLCG_OVERRIDE);
+
+	val = appl_readl(pcie, APPL_CTRL);
+	val |= APPL_CTRL_SYS_PRE_DET_STATE;
+	val |= APPL_CTRL_HW_HOT_RST_EN;
+	appl_writel(pcie, val, APPL_CTRL);
+
+	val = appl_readl(pcie, APPL_CFG_MISC);
+	val |= APPL_CFG_MISC_SLV_EP_MODE;
+	val |= (APPL_CFG_MISC_ARCACHE_VAL << APPL_CFG_MISC_ARCACHE_SHIFT);
+	appl_writel(pcie, val, APPL_CFG_MISC);
+
+	val = appl_readl(pcie, APPL_PINMUX);
+	val |= APPL_PINMUX_CLK_OUTPUT_IN_OVERRIDE_EN;
+	val |= APPL_PINMUX_CLK_OUTPUT_IN_OVERRIDE;
+	appl_writel(pcie, val, APPL_PINMUX);
+
+	appl_writel(pcie, pcie->dbi_res->start & APPL_CFG_BASE_ADDR_MASK,
+		    APPL_CFG_BASE_ADDR);
+
+	appl_writel(pcie, pcie->atu_dma_res->start &
+		    APPL_CFG_IATU_DMA_BASE_ADDR_MASK,
+		    APPL_CFG_IATU_DMA_BASE_ADDR);
+
+	val = appl_readl(pcie, APPL_INTR_EN_L0_0);
+	val |= APPL_INTR_EN_L0_0_SYS_INTR_EN;
+	val |= APPL_INTR_EN_L0_0_LINK_STATE_INT_EN;
+	val |= APPL_INTR_EN_L0_0_PCI_CMD_EN_INT_EN;
+	appl_writel(pcie, val, APPL_INTR_EN_L0_0);
+
+	val = appl_readl(pcie, APPL_INTR_EN_L1_0_0);
+	val |= APPL_INTR_EN_L1_0_0_HOT_RESET_DONE_INT_EN;
+	val |= APPL_INTR_EN_L1_0_0_RDLH_LINK_UP_INT_EN;
+	appl_writel(pcie, val, APPL_INTR_EN_L1_0_0);
+
+	reset_control_deassert(pcie->core_rst);
+
+	if (pcie->update_fc_fixup) {
+		val = dw_pcie_readl_dbi(pci, CFG_TIMER_CTRL_MAX_FUNC_NUM_OFF);
+		val |= 0x1 << CFG_TIMER_CTRL_ACK_NAK_SHIFT;
+		dw_pcie_writel_dbi(pci, CFG_TIMER_CTRL_MAX_FUNC_NUM_OFF, val);
+	}
+
+	config_gen3_gen4_eq_presets(pcie);
+
+	init_host_aspm(pcie);
+
+	/* Disable ASPM-L1SS advertisement if there is no CLKREQ routing */
+	if (!pcie->supports_clkreq) {
+		disable_aspm_l11(pcie);
+		disable_aspm_l12(pcie);
+	}
+
+	val = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
+	val &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
+	dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, val);
+
+	/* Configure N_FTS & FTS */
+	val = dw_pcie_readl_dbi(pci, PORT_LOGIC_ACK_F_ASPM_CTRL);
+	val &= ~(N_FTS_MASK << N_FTS_SHIFT);
+	val |= N_FTS_VAL << N_FTS_SHIFT;
+	dw_pcie_writel_dbi(pci, PORT_LOGIC_ACK_F_ASPM_CTRL, val);
+
+	val = dw_pcie_readl_dbi(pci, PORT_LOGIC_GEN2_CTRL);
+	val &= ~FTS_MASK;
+	val |= FTS_VAL;
+	dw_pcie_writel_dbi(pci, PORT_LOGIC_GEN2_CTRL, val);
+
+	/* Configure Max Speed from DT */
+	if (pcie->max_speed && pcie->max_speed != -EINVAL) {
+		val = dw_pcie_readl_dbi(pci, pcie->pcie_cap_base +
+					PCI_EXP_LNKCAP);
+		val &= ~PCI_EXP_LNKCAP_SLS;
+		val |= pcie->max_speed;
+		dw_pcie_writel_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKCAP,
+				   val);
+	}
+
+	clk_set_rate(pcie->core_clk, GEN4_CORE_CLK_FREQ);
+
+	val = (ep->msi_mem_phys & MSIX_ADDR_MATCH_LOW_OFF_MASK);
+	val |= MSIX_ADDR_MATCH_LOW_OFF_EN;
+	dw_pcie_writel_dbi(pci, MSIX_ADDR_MATCH_LOW_OFF, val);
+	val = (lower_32_bits(ep->msi_mem_phys) & MSIX_ADDR_MATCH_HIGH_OFF_MASK);
+	dw_pcie_writel_dbi(pci, MSIX_ADDR_MATCH_HIGH_OFF, val);
+
+	ret = dw_pcie_ep_init_complete(ep);
+	if (ret) {
+		dev_err(dev, "Failed to complete initialization: %d\n", ret);
+		goto fail_init_complete;
+	}
+
+	dw_pcie_ep_init_notify(ep);
+
+	/* Enable LTSSM */
+	val = appl_readl(pcie, APPL_CTRL);
+	val |= APPL_CTRL_LTSSM_EN;
+	appl_writel(pcie, val, APPL_CTRL);
+
+	pcie->ep_state = EP_STATE_ENABLED;
+	dev_dbg(dev, "Initialization of endpoint is completed\n");
+
+	return;
+
+fail_init_complete:
+	reset_control_assert(pcie->core_rst);
+	tegra_pcie_disable_phy(pcie);
+fail_phy:
+	reset_control_assert(pcie->core_apb_rst);
+fail_core_apb_rst:
+	clk_disable_unprepare(pcie->core_clk);
+fail_core_clk_enable:
+	tegra_pcie_bpmp_set_pll_state(pcie, false);
+fail_pll_init:
+	pm_runtime_put_sync(dev);
+}
+
+static void pex_ep_event_hot_rst_done(struct tegra_pcie_dw *pcie)
+{
+	u32 val = 0;
+
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L0);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_0_0);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_1);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_2);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_3);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_6);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_7);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_8_0);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_9);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_10);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_11);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_13);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_14);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_15);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_INTR_STATUS_L1_17);
+	appl_writel(pcie, 0xFFFFFFFF, APPL_MSI_CTRL_2);
+
+	val = appl_readl(pcie, APPL_CTRL);
+	val |= APPL_CTRL_LTSSM_EN;
+	appl_writel(pcie, val, APPL_CTRL);
+}
+
+static void pex_ep_event_bme_change(struct tegra_pcie_dw *pcie)
+{
+	struct dw_pcie *pci = &pcie->pci;
+	u32 val, speed;
+
+	/* If EP doesn't advertise L1SS, just return */
+	val = dw_pcie_readl_dbi(pci, pcie->cfg_link_cap_l1sub);
+	if (!(val & (PCI_L1SS_CAP_ASPM_L1_1 | PCI_L1SS_CAP_ASPM_L1_2)))
+		return;
+
+	/* Check if BME is set to '1' */
+	val = dw_pcie_readl_dbi(pci, PCI_COMMAND);
+	if (val & PCI_COMMAND_MASTER) {
+		ktime_t timeout;
+
+		/* 110us for both snoop and no-snoop */
+		val = 110 | (2 << PCI_LTR_SCALE_SHIFT) | LTR_MSG_REQ;
+		val |= (val << LTR_MST_NO_SNOOP_SHIFT);
+		appl_writel(pcie, val, APPL_LTR_MSG_1);
+
+		/* Send LTR upstream */
+		val = appl_readl(pcie, APPL_LTR_MSG_2);
+		val |= APPL_LTR_MSG_2_LTR_MSG_REQ_STATE;
+		appl_writel(pcie, val, APPL_LTR_MSG_2);
+
+		timeout = ktime_add_us(ktime_get(), LTR_MSG_TIMEOUT);
+		for (;;) {
+			val = appl_readl(pcie, APPL_LTR_MSG_2);
+			if (!(val & APPL_LTR_MSG_2_LTR_MSG_REQ_STATE))
+				break;
+			if (ktime_after(ktime_get(), timeout))
+				break;
+			usleep_range(1000, 1100);
+		}
+		if (val & APPL_LTR_MSG_2_LTR_MSG_REQ_STATE)
+			dev_err(pcie->dev, "Failed to send LTR message\n");
+	}
+
+	speed = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA) &
+		PCI_EXP_LNKSTA_CLS;
+	clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);
+}
+
+static int tegra_pcie_ep_work_thread(void *p)
+{
+	struct tegra_pcie_dw *pcie = (struct tegra_pcie_dw *)p;
+	u32 event;
+
+	while (true) {
+		wait_event_interruptible(pcie->wq,
+					 !kfifo_is_empty(&pcie->event_fifo));
+
+		if (kthread_should_stop())
+			break;
+
+		if (!kfifo_get(&pcie->event_fifo, &event)) {
+			dev_warn(pcie->dev, "EVENT FIFO is empty\n");
+			continue;
+		}
+
+		switch (event) {
+		case EP_PEX_RST_DEASSERT:
+			dev_info(pcie->dev, "EVENT: EP_PEX_RST_DEASSERT\n");
+			pex_ep_event_pex_rst_deassert(pcie);
+			break;
+
+		case EP_PEX_RST_ASSERT:
+			dev_info(pcie->dev, "EVENT: EP_PEX_RST_ASSERT\n");
+			pex_ep_event_pex_rst_assert(pcie);
+			break;
+
+		case EP_HOT_RST_DONE:
+			dev_info(pcie->dev, "EVENT: EP_HOT_RST_DONE\n");
+			pex_ep_event_hot_rst_done(pcie);
+			break;
+
+		case EP_BME_CHANGE:
+			dev_info(pcie->dev, "EVENT: EP_BME_CHANGE\n");
+			pex_ep_event_bme_change(pcie);
+			break;
+
+		case EP_EVENT_EXIT:
+			dev_info(pcie->dev, "EVENT: EP_EVENT_EXIT\n");
+			return 0;
+
+		default:
+			dev_warn(pcie->dev, "Invalid PCIe EP event\n");
+			break;
+		}
+	}
+
+	return 0;
+}
+
+static irqreturn_t tegra_pcie_ep_pex_rst_irq(int irq, void *arg)
+{
+	struct tegra_pcie_dw *pcie = arg;
+
+	if (gpiod_get_value(pcie->pex_rst_gpiod)) {
+		if (!kfifo_put(&pcie->event_fifo, EP_PEX_RST_ASSERT)) {
+			dev_err(pcie->dev, "EVENT FIFO is full\n");
+			return IRQ_HANDLED;
+		}
+	} else {
+		if (!kfifo_put(&pcie->event_fifo, EP_PEX_RST_DEASSERT)) {
+			dev_err(pcie->dev, "EVENT FIFO is full\n");
+			return IRQ_HANDLED;
+		}
+	}
+
+	wake_up(&pcie->wq);
+
+	return IRQ_HANDLED;
+}
+
+static int tegra_pcie_ep_raise_legacy_irq(struct tegra_pcie_dw *pcie, u16 irq)
+{
+	/* Tegra194 supports only INTA */
+	if (irq > 1)
+		return -EINVAL;
+
+	appl_writel(pcie, 1, APPL_LEGACY_INTX);
+	mdelay(1);
+	appl_writel(pcie, 0, APPL_LEGACY_INTX);
+	return 0;
+}
+
+static int tegra_pcie_ep_raise_msi_irq(struct tegra_pcie_dw *pcie, u16 irq)
+{
+	if (unlikely(irq > 31))
+		return -EINVAL;
+
+	appl_writel(pcie, (1 << irq), APPL_MSI_CTRL_1);
+
+	return 0;
+}
+
+static int tegra_pcie_ep_raise_msix_irq(struct tegra_pcie_dw *pcie, u16 irq)
+{
+	struct dw_pcie_ep *ep = &pcie->pci.ep;
+
+	writel(irq, ep->msi_mem);
+
+	return 0;
+}
+
+static int tegra_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
+				   enum pci_epc_irq_type type,
+				   u16 interrupt_num)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	struct tegra_pcie_dw *pcie = to_tegra_pcie(pci);
+
+	switch (type) {
+	case PCI_EPC_IRQ_LEGACY:
+		return tegra_pcie_ep_raise_legacy_irq(pcie, interrupt_num);
+
+	case PCI_EPC_IRQ_MSI:
+		return tegra_pcie_ep_raise_msi_irq(pcie, interrupt_num);
+
+	case PCI_EPC_IRQ_MSIX:
+		return tegra_pcie_ep_raise_msix_irq(pcie, interrupt_num);
+
+	default:
+		dev_err(pci->dev, "Unknown IRQ type\n");
+		return -EPERM;
+	}
+
+	return 0;
+}
+
+static const struct pci_epc_features tegra_pcie_epc_features = {
+	.linkup_notifier = true,
+	.msi_capable = false,
+	.msix_capable = false,
+	.reserved_bar = 1 << BAR_2 | 1 << BAR_3 | 1 << BAR_4 | 1 << BAR_5,
+	.bar_fixed_64bit = 1 << BAR_0,
+	.bar_fixed_size[0] = SZ_1M,
+	.skip_core_init = true,
+};
+
+static const struct pci_epc_features*
+tegra_pcie_ep_get_features(struct dw_pcie_ep *ep)
+{
+	return &tegra_pcie_epc_features;
+}
+
+static struct dw_pcie_ep_ops pcie_ep_ops = {
+	.raise_irq = tegra_pcie_ep_raise_irq,
+	.get_features = tegra_pcie_ep_get_features,
+};
+
+static int tegra_pcie_config_ep(struct tegra_pcie_dw *pcie,
+				struct platform_device *pdev)
+{
+	struct dw_pcie *pci = &pcie->pci;
+	struct device *dev = pcie->dev;
+	struct dw_pcie_ep *ep;
+	struct resource *res;
+	char *name;
+	int ret;
+
+	ep = &pci->ep;
+	ep->ops = &pcie_ep_ops;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
+	if (!res)
+		return -EINVAL;
+
+	ep->phys_base = res->start;
+	ep->addr_size = resource_size(res);
+	ep->page_size = SZ_64K;
+
+	ret = gpiod_set_debounce(pcie->pex_rst_gpiod, PERST_DEBOUNCE_TIME);
+	if (ret < 0) {
+		dev_err(dev, "Failed to set PERST GPIO debounce time: %d\n",
+			ret);
+		return ret;
+	}
+
+	ret = gpiod_to_irq(pcie->pex_rst_gpiod);
+	if (ret < 0) {
+		dev_err(dev, "Failed to get IRQ for PERST GPIO: %d\n", ret);
+		return ret;
+	}
+	pcie->pex_rst_irq = (unsigned int)ret;
+
+	name = devm_kasprintf(dev, GFP_KERNEL, "tegra_pcie_%u_pex_rst_irq",
+			      pcie->cid);
+	if (!name) {
+		dev_err(dev, "Failed to create PERST IRQ string\n");
+		return -ENOMEM;
+	}
+	ret = devm_request_irq(dev, pcie->pex_rst_irq,
+			       tegra_pcie_ep_pex_rst_irq,
+			       IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+			       name, (void *)pcie);
+	if (ret < 0) {
+		dev_err(dev, "Failed to request IRQ for PERST: %d\n", ret);
+		return ret;
+	}
+	disable_irq(pcie->pex_rst_irq);
+
+	pcie->ep_state = EP_STATE_DISABLED;
+
+	INIT_KFIFO(pcie->event_fifo);
+
+	init_waitqueue_head(&pcie->wq);
+
+	name = devm_kasprintf(dev, GFP_KERNEL, "tegra_pcie_%u_ep_work",
+			      pcie->cid);
+	if (!name) {
+		dev_err(dev, "Failed to create PCIe EP work thread string\n");
+		return -ENOMEM;
+	}
+	pcie->pcie_ep_task = kthread_run(tegra_pcie_ep_work_thread,
+					 (void *)pcie, name);
+	if (IS_ERR(pcie->pcie_ep_task)) {
+		dev_err(dev, "Failed to create PCIe EP work thread: %d\n",
+			PTR_ERR_OR_ZERO(pcie->pcie_ep_task));
+		return PTR_ERR_OR_ZERO(pcie->pcie_ep_task);
+	}
+
+	pm_runtime_enable(dev);
+
+	ret = dw_pcie_ep_init(ep);
+	if (ret) {
+		dev_err(dev, "Failed to initialize DWC Endpoint subsystem: %d\n",
+			ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct tegra_pcie_dw_of_data tegra_pcie_dw_rc_of_data = {
+	.mode = DW_PCIE_RC_TYPE,
+};
+
+static const struct tegra_pcie_dw_of_data tegra_pcie_dw_ep_of_data = {
+	.mode = DW_PCIE_EP_TYPE,
+};
+
+static const struct of_device_id tegra_pcie_dw_of_match[] = {
+	{
+		.compatible = "nvidia,tegra194-pcie",
+		.data = &tegra_pcie_dw_rc_of_data,
+	},
+	{
+		.compatible = "nvidia,tegra194-pcie-ep",
+		.data = &tegra_pcie_dw_ep_of_data,
+	},
+	{},
+};
+
 static int tegra_pcie_dw_probe(struct platform_device *pdev)
 {
+	const struct tegra_pcie_dw_of_data *data;
+	const struct of_device_id *match;
 	struct device *dev = &pdev->dev;
 	struct resource *atu_dma_res;
 	struct tegra_pcie_dw *pcie;
@@ -1440,6 +2152,12 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev)
 	int ret;
 	u32 i;
 
+	match = of_match_device(of_match_ptr(tegra_pcie_dw_of_match), dev);
+	if (!match)
+		return -EINVAL;
+
+	data = (struct tegra_pcie_dw_of_data *)match->data;
+
 	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
 	if (!pcie)
 		return -ENOMEM;
@@ -1449,6 +2167,7 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev)
 	pci->ops = &tegra_dw_pcie_ops;
 	pp = &pci->pp;
 	pcie->dev = &pdev->dev;
+	pcie->mode = (enum dw_pcie_device_mode)data->mode;
 
 	ret = tegra_pcie_dw_parse_dt(pcie);
 	if (ret < 0) {
@@ -1570,11 +2289,24 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, pcie);
 
-	ret = tegra_pcie_config_rp(pcie);
-	if (ret && ret != -ENOMEDIUM)
-		goto fail;
-	else
-		return 0;
+	switch (pcie->mode) {
+	case DW_PCIE_RC_TYPE:
+		ret = tegra_pcie_config_rp(pcie);
+		if (ret && ret != -ENOMEDIUM)
+			goto fail;
+		else
+			return 0;
+		break;
+
+	case DW_PCIE_EP_TYPE:
+		ret = tegra_pcie_config_ep(pcie, pdev);
+		if (ret < 0)
+			goto fail;
+		break;
+
+	default:
+		dev_err(dev, "Invalid PCIe device type %d\n", pcie->mode);
+	}
 
 fail:
 	tegra_bpmp_put(pcie->bpmp);
@@ -1697,13 +2429,6 @@ static void tegra_pcie_dw_shutdown(struct platform_device *pdev)
 	__deinit_controller(pcie);
 }
 
-static const struct of_device_id tegra_pcie_dw_of_match[] = {
-	{
-		.compatible = "nvidia,tegra194-pcie",
-	},
-	{},
-};
-
 static const struct dev_pm_ops tegra_pcie_dw_pm_ops = {
 	.suspend_late = tegra_pcie_dw_suspend_late,
 	.suspend_noirq = tegra_pcie_dw_suspend_noirq,
-- 
2.17.1

Re: [PATCH 3/6] PCI: tegra: Add support for PCIe endpoint mode in Tegra194

[Bjorn Helgaas]

On Fri, Nov 22, 2019 at 04:15:02PM +0530, Vidya Sagar wrote:
> Add support for the endpoint mode of Synopsys DesignWare core based
> dual mode PCIe controllers present in Tegra194 SoC.

> +static irqreturn_t tegra_pcie_ep_irq_handler(struct tegra_pcie_dw *pcie)
> +{
> +	struct dw_pcie_ep *ep = &pcie->pci.ep;
> +	u32 val, tmp;
> +
> +	val = appl_readl(pcie, APPL_INTR_STATUS_L0);
> +	if (val & APPL_INTR_STATUS_L0_LINK_STATE_INT) {
> +		val = appl_readl(pcie, APPL_INTR_STATUS_L1_0_0);
> +		appl_writel(pcie, val, APPL_INTR_STATUS_L1_0_0);
> +		if (val & APPL_INTR_STATUS_L1_0_0_HOT_RESET_DONE) {
> +			/* clear any stale PEX_RST interrupt */
> +			if (!kfifo_put(&pcie->event_fifo, EP_HOT_RST_DONE)) {
> +				dev_err(pcie->dev, "EVENT FIFO is full\n");
> +				return IRQ_HANDLED;
> +			}
> +			wake_up(&pcie->wq);
> +		}
> +		if (val & APPL_INTR_STATUS_L1_0_0_RDLH_LINK_UP_CHGED) {
> +			tmp = appl_readl(pcie, APPL_LINK_STATUS);
> +			if (tmp & APPL_LINK_STATUS_RDLH_LINK_UP) {
> +				dev_info(pcie->dev, "Link is up with Host\n");
> +				dw_pcie_ep_linkup(ep);
> +			}
> +		}
> +	} else if (val & APPL_INTR_STATUS_L0_PCI_CMD_EN_INT) {

Is it really the case that only one of
APPL_INTR_STATUS_L0_LINK_STATE_INT and
APPL_INTR_STATUS_L0_PCI_CMD_EN_INT can be set?

If it's possible that both could be set, maybe this should be
something like this?

  int spurious = 1;

  if (val & APPL_INTR_STATUS_L0_LINK_STATE_INT) {
    ...
    spurious = 0;
  }
  if (val & APPL_INTR_STATUS_L0_PCI_CMD_EN_INT) {
    ...
    spurious = 0;
  }

  if (spurious) {
    dev_warn(...)
  }

> +		val = appl_readl(pcie, APPL_INTR_STATUS_L1_15);
> +		appl_writel(pcie, val, APPL_INTR_STATUS_L1_15);
> +		if (val & APPL_INTR_STATUS_L1_15_CFG_BME_CHGED) {
> +			if (!kfifo_put(&pcie->event_fifo, EP_BME_CHANGE)) {
> +				dev_err(pcie->dev, "EVENT FIFO is full\n");
> +				return IRQ_HANDLED;
> +			}
> +			wake_up(&pcie->wq);
> +		}
> +	} else {
> +		dev_warn(pcie->dev, "Random interrupt (STATUS = 0x%08X)\n",
> +			 val);
> +		appl_writel(pcie, val, APPL_INTR_STATUS_L0);
> +	}
> +
> +	return IRQ_HANDLED;
> +}

> +static int tegra_pcie_ep_work_thread(void *p)
> +{
> +	struct tegra_pcie_dw *pcie = (struct tegra_pcie_dw *)p;
> +	u32 event;
> +
> +	while (true) {
> +		wait_event_interruptible(pcie->wq,
> +					 !kfifo_is_empty(&pcie->event_fifo));
> +
> +		if (kthread_should_stop())
> +			break;
> +
> +		if (!kfifo_get(&pcie->event_fifo, &event)) {
> +			dev_warn(pcie->dev, "EVENT FIFO is empty\n");
> +			continue;
> +		}
> +
> +		switch (event) {
> +		case EP_PEX_RST_DEASSERT:
> +			dev_info(pcie->dev, "EVENT: EP_PEX_RST_DEASSERT\n");
> +			pex_ep_event_pex_rst_deassert(pcie);
> +			break;
> +
> +		case EP_PEX_RST_ASSERT:
> +			dev_info(pcie->dev, "EVENT: EP_PEX_RST_ASSERT\n");
> +			pex_ep_event_pex_rst_assert(pcie);
> +			break;
> +
> +		case EP_HOT_RST_DONE:
> +			dev_info(pcie->dev, "EVENT: EP_HOT_RST_DONE\n");
> +			pex_ep_event_hot_rst_done(pcie);
> +			break;
> +
> +		case EP_BME_CHANGE:
> +			dev_info(pcie->dev, "EVENT: EP_BME_CHANGE\n");
> +			pex_ep_event_bme_change(pcie);
> +			break;
> +
> +		case EP_EVENT_EXIT:
> +			dev_info(pcie->dev, "EVENT: EP_EVENT_EXIT\n");
> +			return 0;
> +
> +		default:
> +			dev_warn(pcie->dev, "Invalid PCIe EP event\n");

Maybe include the invalid event value in the message?

> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}

Re: [PATCH 3/6] PCI: tegra: Add support for PCIe endpoint mode in Tegra194

[Vidya Sagar]

On 11/27/2019 3:07 AM, Bjorn Helgaas wrote:
> On Fri, Nov 22, 2019 at 04:15:02PM +0530, Vidya Sagar wrote:
>> Add support for the endpoint mode of Synopsys DesignWare core based
>> dual mode PCIe controllers present in Tegra194 SoC.
> 
>> +static irqreturn_t tegra_pcie_ep_irq_handler(struct tegra_pcie_dw *pcie)
>> +{
>> +	struct dw_pcie_ep *ep = &pcie->pci.ep;
>> +	u32 val, tmp;
>> +
>> +	val = appl_readl(pcie, APPL_INTR_STATUS_L0);
>> +	if (val & APPL_INTR_STATUS_L0_LINK_STATE_INT) {
>> +		val = appl_readl(pcie, APPL_INTR_STATUS_L1_0_0);
>> +		appl_writel(pcie, val, APPL_INTR_STATUS_L1_0_0);
>> +		if (val & APPL_INTR_STATUS_L1_0_0_HOT_RESET_DONE) {
>> +			/* clear any stale PEX_RST interrupt */
>> +			if (!kfifo_put(&pcie->event_fifo, EP_HOT_RST_DONE)) {
>> +				dev_err(pcie->dev, "EVENT FIFO is full\n");
>> +				return IRQ_HANDLED;
>> +			}
>> +			wake_up(&pcie->wq);
>> +		}
>> +		if (val & APPL_INTR_STATUS_L1_0_0_RDLH_LINK_UP_CHGED) {
>> +			tmp = appl_readl(pcie, APPL_LINK_STATUS);
>> +			if (tmp & APPL_LINK_STATUS_RDLH_LINK_UP) {
>> +				dev_info(pcie->dev, "Link is up with Host\n");
>> +				dw_pcie_ep_linkup(ep);
>> +			}
>> +		}
>> +	} else if (val & APPL_INTR_STATUS_L0_PCI_CMD_EN_INT) {
> 
> Is it really the case that only one of
> APPL_INTR_STATUS_L0_LINK_STATE_INT and
> APPL_INTR_STATUS_L0_PCI_CMD_EN_INT can be set?
Not really.

> 
> If it's possible that both could be set, maybe this should be
> something like this?
> 
>    int spurious = 1;
> 
>    if (val & APPL_INTR_STATUS_L0_LINK_STATE_INT) {
>      ...
>      spurious = 0;
>    }
>    if (val & APPL_INTR_STATUS_L0_PCI_CMD_EN_INT) {
>      ...
>      spurious = 0;
>    }
> 
>    if (spurious) {
>      dev_warn(...)
>    }
I'll take care of this in the next patch series.

> 
>> +		val = appl_readl(pcie, APPL_INTR_STATUS_L1_15);
>> +		appl_writel(pcie, val, APPL_INTR_STATUS_L1_15);
>> +		if (val & APPL_INTR_STATUS_L1_15_CFG_BME_CHGED) {
>> +			if (!kfifo_put(&pcie->event_fifo, EP_BME_CHANGE)) {
>> +				dev_err(pcie->dev, "EVENT FIFO is full\n");
>> +				return IRQ_HANDLED;
>> +			}
>> +			wake_up(&pcie->wq);
>> +		}
>> +	} else {
>> +		dev_warn(pcie->dev, "Random interrupt (STATUS = 0x%08X)\n",
>> +			 val);
>> +		appl_writel(pcie, val, APPL_INTR_STATUS_L0);
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
> 
>> +static int tegra_pcie_ep_work_thread(void *p)
>> +{
>> +	struct tegra_pcie_dw *pcie = (struct tegra_pcie_dw *)p;
>> +	u32 event;
>> +
>> +	while (true) {
>> +		wait_event_interruptible(pcie->wq,
>> +					 !kfifo_is_empty(&pcie->event_fifo));
>> +
>> +		if (kthread_should_stop())
>> +			break;
>> +
>> +		if (!kfifo_get(&pcie->event_fifo, &event)) {
>> +			dev_warn(pcie->dev, "EVENT FIFO is empty\n");
>> +			continue;
>> +		}
>> +
>> +		switch (event) {
>> +		case EP_PEX_RST_DEASSERT:
>> +			dev_info(pcie->dev, "EVENT: EP_PEX_RST_DEASSERT\n");
>> +			pex_ep_event_pex_rst_deassert(pcie);
>> +			break;
>> +
>> +		case EP_PEX_RST_ASSERT:
>> +			dev_info(pcie->dev, "EVENT: EP_PEX_RST_ASSERT\n");
>> +			pex_ep_event_pex_rst_assert(pcie);
>> +			break;
>> +
>> +		case EP_HOT_RST_DONE:
>> +			dev_info(pcie->dev, "EVENT: EP_HOT_RST_DONE\n");
>> +			pex_ep_event_hot_rst_done(pcie);
>> +			break;
>> +
>> +		case EP_BME_CHANGE:
>> +			dev_info(pcie->dev, "EVENT: EP_BME_CHANGE\n");
>> +			pex_ep_event_bme_change(pcie);
>> +			break;
>> +
>> +		case EP_EVENT_EXIT:
>> +			dev_info(pcie->dev, "EVENT: EP_EVENT_EXIT\n");
>> +			return 0;
>> +
>> +		default:
>> +			dev_warn(pcie->dev, "Invalid PCIe EP event\n");
> 
> Maybe include the invalid event value in the message?
I'll take care of this in the next patch series

> 
>> +			break;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}

[PATCH 4/6] arm64: tegra: Add PCIe endpoint controllers nodes for Tegra194

[Vidya Sagar]

Add endpoint mode controllers nodes for the dual mode PCIe controllers
present in Tegra194 SoC.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
 arch/arm64/boot/dts/nvidia/tegra194.dtsi | 99 ++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra194.dtsi b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
index 11220d97adb8..2c8fae854a23 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra194.dtsi
@@ -1425,6 +1425,105 @@
 			  0x82000000 0x0  0x40000000 0x1f 0x40000000 0x0 0xc0000000>; /* non-prefetchable memory (3GB) */
 	};
 
+	pcie_ep@14160000 {
+		compatible = "nvidia,tegra194-pcie-ep", "snps,dw-pcie-ep";
+		power-domains = <&bpmp TEGRA194_POWER_DOMAIN_PCIEX4A>;
+		reg = <0x00 0x14160000 0x0 0x00020000   /* appl registers (128K)      */
+		       0x00 0x36040000 0x0 0x00040000   /* iATU_DMA reg space (256K)  */
+		       0x00 0x36080000 0x0 0x00040000   /* DBI reg space (256K)       */
+		       0x14 0x00000000 0x4 0x00000000>; /* Address Space (16G)        */
+		reg-names = "appl", "atu_dma", "dbi", "addr_space";
+
+		status = "disabled";
+
+		num-lanes = <4>;
+		num-ib-windows = <2>;
+		num-ob-windows = <8>;
+
+		clocks = <&bpmp TEGRA194_CLK_PEX0_CORE_4>;
+		clock-names = "core";
+
+		resets = <&bpmp TEGRA194_RESET_PEX0_CORE_4_APB>,
+			 <&bpmp TEGRA194_RESET_PEX0_CORE_4>;
+		reset-names = "apb", "core";
+
+		interrupts = <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH>;	/* controller interrupt */
+		interrupt-names = "intr";
+
+		nvidia,bpmp = <&bpmp 4>;
+
+		nvidia,aspm-cmrt-us = <60>;
+		nvidia,aspm-pwr-on-t-us = <20>;
+		nvidia,aspm-l0s-entrance-latency-us = <3>;
+	};
+
+	pcie_ep@14180000 {
+		compatible = "nvidia,tegra194-pcie-ep", "snps,dw-pcie-ep";
+		power-domains = <&bpmp TEGRA194_POWER_DOMAIN_PCIEX8B>;
+		reg = <0x00 0x14180000 0x0 0x00020000   /* appl registers (128K)      */
+		       0x00 0x38040000 0x0 0x00040000   /* iATU_DMA reg space (256K)  */
+		       0x00 0x38080000 0x0 0x00040000   /* DBI reg space (256K)       */
+		       0x18 0x00000000 0x4 0x00000000>; /* Address Space (16G)        */
+		reg-names = "appl", "atu_dma", "dbi", "addr_space";
+
+		status = "disabled";
+
+		num-lanes = <8>;
+		num-ib-windows = <2>;
+		num-ob-windows = <8>;
+
+		clocks = <&bpmp TEGRA194_CLK_PEX0_CORE_0>;
+		clock-names = "core";
+
+		resets = <&bpmp TEGRA194_RESET_PEX0_CORE_0_APB>,
+			 <&bpmp TEGRA194_RESET_PEX0_CORE_0>;
+		reset-names = "apb", "core";
+
+		interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;	/* controller interrupt */
+		interrupt-names = "intr";
+
+		nvidia,bpmp = <&bpmp 0>;
+
+		nvidia,aspm-cmrt-us = <60>;
+		nvidia,aspm-pwr-on-t-us = <20>;
+		nvidia,aspm-l0s-entrance-latency-us = <3>;
+	};
+
+	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";
+
+		status = "disabled";
+
+		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>;
+	};
+
 	sysram@40000000 {
 		compatible = "nvidia,tegra194-sysram", "mmio-sram";
 		reg = <0x0 0x40000000 0x0 0x50000>;
-- 
2.17.1

[PATCH 5/6] arm64: tegra: Enable GPIO controllers nodes for P2972-0000 platform

[Vidya Sagar]

Enable GPIO controllers nodes for P2972-0000 platform which are required
by other controllers in the SoC for example when PCIe C5 controller
operates in endpoint mode.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
 arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
index 353a6a22196d..7eb64b816e08 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
+++ b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
@@ -37,6 +37,14 @@
 			status = "okay";
 		};
 
+		gpio@2200000 {
+			status = "okay";
+		};
+
+		gpio@c2f0000 {
+			status = "okay";
+		};
+
 		pwm@c340000 {
 			status = "okay";
 		};
-- 
2.17.1

Re: [PATCH 5/6] arm64: tegra: Enable GPIO controllers nodes for P2972-0000 platform

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1051 bytes --]

On Fri, Nov 22, 2019 at 04:15:04PM +0530, Vidya Sagar wrote:
> Enable GPIO controllers nodes for P2972-0000 platform which are required
> by other controllers in the SoC for example when PCIe C5 controller
> operates in endpoint mode.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
>  arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts | 8 ++++++++
>  1 file changed, 8 insertions(+)

The GPIO controllers are enabled by default, so there's no need to
explicitly enable them.

Thierry

> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
> index 353a6a22196d..7eb64b816e08 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
> +++ b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
> @@ -37,6 +37,14 @@
>  			status = "okay";
>  		};
>  
> +		gpio@2200000 {
> +			status = "okay";
> +		};
> +
> +		gpio@c2f0000 {
> +			status = "okay";
> +		};
> +
>  		pwm@c340000 {
>  			status = "okay";
>  		};
> -- 
> 2.17.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/6] arm64: tegra: Enable GPIO controllers nodes for P2972-0000 platform

[Vidya Sagar]

On 11/22/2019 6:50 PM, Thierry Reding wrote:
> On Fri, Nov 22, 2019 at 04:15:04PM +0530, Vidya Sagar wrote:
>> Enable GPIO controllers nodes for P2972-0000 platform which are required
>> by other controllers in the SoC for example when PCIe C5 controller
>> operates in endpoint mode.
>>
>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>> ---
>>   arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts | 8 ++++++++
>>   1 file changed, 8 insertions(+)
> 
> The GPIO controllers are enabled by default, so there's no need to
> explicitly enable them.
Yup. I'll remove this patch.
Since I didn't see 'status' entry in GPIO nodes in tegra194.dtsi file, I thought
it means 'disabled'. But, contrary to my understanding, absence of 'status' entry
seems to mean 'okay'.

- Vidya Sagar
> 
> Thierry
> 
>>
>> diff --git a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
>> index 353a6a22196d..7eb64b816e08 100644
>> --- a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
>> +++ b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
>> @@ -37,6 +37,14 @@
>>   			status = "okay";
>>   		};
>>   
>> +		gpio@2200000 {
>> +			status = "okay";
>> +		};
>> +
>> +		gpio@c2f0000 {
>> +			status = "okay";
>> +		};
>> +
>>   		pwm@c340000 {
>>   			status = "okay";
>>   		};
>> -- 
>> 2.17.1
>>

[PATCH 6/6] arm64: tegra: Add support for PCIe endpoint mode in P2972-0000 platform

[Vidya Sagar]

Add endpoint mode support for PCIe C5 controller in P2972-0000 platform
with information about supplies, PHY, PERST GPIO and GPIO that controls
PCIe reference clock coming from the host system.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
 .../boot/dts/nvidia/tegra194-p2972-0000.dts   | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
index 7eb64b816e08..58c3a9677bc8 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
+++ b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
@@ -43,6 +43,19 @@
 
 		gpio@c2f0000 {
 			status = "okay";
+			/*
+			 * Change the below node's status to 'okay' when
+			 * PCIe C5 controller is enabled to operate in endpoint
+			 * to allow REFCLK from the host system to flow into
+			 * the controller.
+			 */
+			pex-refclk-sel-high {
+				gpio-hog;
+				output-high;
+				gpios = <TEGRA194_AON_GPIO(AA, 5) 0>;
+				label = "pex_refclk_sel_high";
+				status = "disabled";
+			};
 		};
 
 		pwm@c340000 {
@@ -144,6 +157,22 @@
 			    "p2u-5", "p2u-6", "p2u-7";
 	};
 
+	pcie_ep@141a0000 {
+		status = "disabled";
+
+		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";
+	};
+
 	fan: fan {
 		compatible = "pwm-fan";
 		pwms = <&pwm4 0 45334>;
-- 
2.17.1

Re: [PATCH 6/6] arm64: tegra: Add support for PCIe endpoint mode in P2972-0000 platform

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2114 bytes --]

On Fri, Nov 22, 2019 at 04:15:05PM +0530, Vidya Sagar wrote:
> Add endpoint mode support for PCIe C5 controller in P2972-0000 platform
> with information about supplies, PHY, PERST GPIO and GPIO that controls
> PCIe reference clock coming from the host system.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
>  .../boot/dts/nvidia/tegra194-p2972-0000.dts   | 29 +++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
> index 7eb64b816e08..58c3a9677bc8 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
> +++ b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
> @@ -43,6 +43,19 @@
>  
>  		gpio@c2f0000 {
>  			status = "okay";
> +			/*
> +			 * Change the below node's status to 'okay' when
> +			 * PCIe C5 controller is enabled to operate in endpoint
> +			 * to allow REFCLK from the host system to flow into
> +			 * the controller.
> +			 */
> +			pex-refclk-sel-high {
> +				gpio-hog;
> +				output-high;
> +				gpios = <TEGRA194_AON_GPIO(AA, 5) 0>;
> +				label = "pex_refclk_sel_high";
> +				status = "disabled";
> +			};

Why don't we put this into the PCIe controller's node as a reference to
that GPIO? Seems like the controller would know exactly when this pin
needs to go high or low, so why does it have to be a hog?

Thierry

>  		};
>  
>  		pwm@c340000 {
> @@ -144,6 +157,22 @@
>  			    "p2u-5", "p2u-6", "p2u-7";
>  	};
>  
> +	pcie_ep@141a0000 {
> +		status = "disabled";
> +
> +		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";
> +	};
> +
>  	fan: fan {
>  		compatible = "pwm-fan";
>  		pwms = <&pwm4 0 45334>;
> -- 
> 2.17.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 6/6] arm64: tegra: Add support for PCIe endpoint mode in P2972-0000 platform

[Vidya Sagar]

On 11/22/2019 6:55 PM, Thierry Reding wrote:
> On Fri, Nov 22, 2019 at 04:15:05PM +0530, Vidya Sagar wrote:
>> Add endpoint mode support for PCIe C5 controller in P2972-0000 platform
>> with information about supplies, PHY, PERST GPIO and GPIO that controls
>> PCIe reference clock coming from the host system.
>>
>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>> ---
>>   .../boot/dts/nvidia/tegra194-p2972-0000.dts   | 29 +++++++++++++++++++
>>   1 file changed, 29 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
>> index 7eb64b816e08..58c3a9677bc8 100644
>> --- a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
>> +++ b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
>> @@ -43,6 +43,19 @@
>>   
>>   		gpio@c2f0000 {
>>   			status = "okay";
>> +			/*
>> +			 * Change the below node's status to 'okay' when
>> +			 * PCIe C5 controller is enabled to operate in endpoint
>> +			 * to allow REFCLK from the host system to flow into
>> +			 * the controller.
>> +			 */
>> +			pex-refclk-sel-high {
>> +				gpio-hog;
>> +				output-high;
>> +				gpios = <TEGRA194_AON_GPIO(AA, 5) 0>;
>> +				label = "pex_refclk_sel_high";
>> +				status = "disabled";
>> +			};
> 
> Why don't we put this into the PCIe controller's node as a reference to
> that GPIO? Seems like the controller would know exactly when this pin
> needs to go high or low, so why does it have to be a hog?
> 
> Thierry
Are you saying something like 'nvidia,enable-refclk-in'?
I was thinking, since this is like a board level configuration specific to Jetson-Xavier,
it would suffice to just hog it according to the mode of operation of PCIe controller.
But, I see one advantage of referencing it in the PCIe node (so that the driver can configure
it as and when needed) is that one has to be careful just to enable either PCIe RP or EP
node and not worry about other thing (like this).
Let me know if I got this right.

- Vidya Sagar

> 
>>   		};
>>   
>>   		pwm@c340000 {
>> @@ -144,6 +157,22 @@
>>   			    "p2u-5", "p2u-6", "p2u-7";
>>   	};
>>   
>> +	pcie_ep@141a0000 {
>> +		status = "disabled";
>> +
>> +		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";
>> +	};
>> +
>>   	fan: fan {
>>   		compatible = "pwm-fan";
>>   		pwms = <&pwm4 0 45334>;
>> -- 
>> 2.17.1
>>

Re: [PATCH 6/6] arm64: tegra: Add support for PCIe endpoint mode in P2972-0000 platform

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3582 bytes --]

On Mon, Nov 25, 2019 at 12:30:53PM +0530, Vidya Sagar wrote:
> On 11/22/2019 6:55 PM, Thierry Reding wrote:
> > On Fri, Nov 22, 2019 at 04:15:05PM +0530, Vidya Sagar wrote:
> > > Add endpoint mode support for PCIe C5 controller in P2972-0000 platform
> > > with information about supplies, PHY, PERST GPIO and GPIO that controls
> > > PCIe reference clock coming from the host system.
> > > 
> > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > > ---
> > >   .../boot/dts/nvidia/tegra194-p2972-0000.dts   | 29 +++++++++++++++++++
> > >   1 file changed, 29 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
> > > index 7eb64b816e08..58c3a9677bc8 100644
> > > --- a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
> > > +++ b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
> > > @@ -43,6 +43,19 @@
> > >   		gpio@c2f0000 {
> > >   			status = "okay";
> > > +			/*
> > > +			 * Change the below node's status to 'okay' when
> > > +			 * PCIe C5 controller is enabled to operate in endpoint
> > > +			 * to allow REFCLK from the host system to flow into
> > > +			 * the controller.
> > > +			 */
> > > +			pex-refclk-sel-high {
> > > +				gpio-hog;
> > > +				output-high;
> > > +				gpios = <TEGRA194_AON_GPIO(AA, 5) 0>;
> > > +				label = "pex_refclk_sel_high";
> > > +				status = "disabled";
> > > +			};
> > 
> > Why don't we put this into the PCIe controller's node as a reference to
> > that GPIO? Seems like the controller would know exactly when this pin
> > needs to go high or low, so why does it have to be a hog?
> > 
> > Thierry
> Are you saying something like 'nvidia,enable-refclk-in'?
> I was thinking, since this is like a board level configuration specific to Jetson-Xavier,
> it would suffice to just hog it according to the mode of operation of PCIe controller.
> But, I see one advantage of referencing it in the PCIe node (so that the driver can configure
> it as and when needed) is that one has to be careful just to enable either PCIe RP or EP
> node and not worry about other thing (like this).
> Let me know if I got this right.

Yeah, that's exactly why I think referencing this from the controller
and controlling it in the driver is preferable.

If this is some sort of select signal I think it makes sense to name it
"nvidia,refclk-select-gpios" or something. Does this appear in the
schematic somewhere? Or does the IP have a name for this? Those are
usually good places to look for inspiration on the name because it's
what hardware designers will be familiar with and they are technically
the ones who should write the DT, even if that's rarely the case.

Thierry

> 
> - Vidya Sagar
> 
> > 
> > >   		};
> > >   		pwm@c340000 {
> > > @@ -144,6 +157,22 @@
> > >   			    "p2u-5", "p2u-6", "p2u-7";
> > >   	};
> > > +	pcie_ep@141a0000 {
> > > +		status = "disabled";
> > > +
> > > +		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";
> > > +	};
> > > +
> > >   	fan: fan {
> > >   		compatible = "pwm-fan";
> > >   		pwms = <&pwm4 0 45334>;
> > > -- 
> > > 2.17.1
> > > 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 6/6] arm64: tegra: Add support for PCIe endpoint mode in P2972-0000 platform

[Vidya Sagar]

On 11/25/2019 12:55 PM, Thierry Reding wrote:
> On Mon, Nov 25, 2019 at 12:30:53PM +0530, Vidya Sagar wrote:
>> On 11/22/2019 6:55 PM, Thierry Reding wrote:
>>> On Fri, Nov 22, 2019 at 04:15:05PM +0530, Vidya Sagar wrote:
>>>> Add endpoint mode support for PCIe C5 controller in P2972-0000 platform
>>>> with information about supplies, PHY, PERST GPIO and GPIO that controls
>>>> PCIe reference clock coming from the host system.
>>>>
>>>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>>>> ---
>>>>    .../boot/dts/nvidia/tegra194-p2972-0000.dts   | 29 +++++++++++++++++++
>>>>    1 file changed, 29 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
>>>> index 7eb64b816e08..58c3a9677bc8 100644
>>>> --- a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
>>>> +++ b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
>>>> @@ -43,6 +43,19 @@
>>>>    		gpio@c2f0000 {
>>>>    			status = "okay";
>>>> +			/*
>>>> +			 * Change the below node's status to 'okay' when
>>>> +			 * PCIe C5 controller is enabled to operate in endpoint
>>>> +			 * to allow REFCLK from the host system to flow into
>>>> +			 * the controller.
>>>> +			 */
>>>> +			pex-refclk-sel-high {
>>>> +				gpio-hog;
>>>> +				output-high;
>>>> +				gpios = <TEGRA194_AON_GPIO(AA, 5) 0>;
>>>> +				label = "pex_refclk_sel_high";
>>>> +				status = "disabled";
>>>> +			};
>>>
>>> Why don't we put this into the PCIe controller's node as a reference to
>>> that GPIO? Seems like the controller would know exactly when this pin
>>> needs to go high or low, so why does it have to be a hog?
>>>
>>> Thierry
>> Are you saying something like 'nvidia,enable-refclk-in'?
>> I was thinking, since this is like a board level configuration specific to Jetson-Xavier,
>> it would suffice to just hog it according to the mode of operation of PCIe controller.
>> But, I see one advantage of referencing it in the PCIe node (so that the driver can configure
>> it as and when needed) is that one has to be careful just to enable either PCIe RP or EP
>> node and not worry about other thing (like this).
>> Let me know if I got this right.
> 
> Yeah, that's exactly why I think referencing this from the controller
> and controlling it in the driver is preferable.
> 
> If this is some sort of select signal I think it makes sense to name it
> "nvidia,refclk-select-gpios" or something. Does this appear in the
> schematic somewhere? Or does the IP have a name for this? Those are
> usually good places to look for inspiration on the name because it's
> what hardware designers will be familiar with and they are technically
> the ones who should write the DT, even if that's rarely the case.
Schematic has "PEX_REFCLK_SEL" name.
I would go with 'nvidia,refclk-select-gpios' and make the change.

- Vidya Sagar
> 
> Thierry
> 
>>
>> - Vidya Sagar
>>
>>>
>>>>    		};
>>>>    		pwm@c340000 {
>>>> @@ -144,6 +157,22 @@
>>>>    			    "p2u-5", "p2u-6", "p2u-7";
>>>>    	};
>>>> +	pcie_ep@141a0000 {
>>>> +		status = "disabled";
>>>> +
>>>> +		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";
>>>> +	};
>>>> +
>>>>    	fan: fan {
>>>>    		compatible = "pwm-fan";
>>>>    		pwms = <&pwm4 0 45334>;
>>>> -- 
>>>> 2.17.1
>>>>
>>

Re: [PATCH 6/6] arm64: tegra: Add support for PCIe endpoint mode in P2972-0000 platform

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4792 bytes --]

On Mon, Nov 25, 2019 at 01:03:27PM +0530, Vidya Sagar wrote:
> On 11/25/2019 12:55 PM, Thierry Reding wrote:
> > On Mon, Nov 25, 2019 at 12:30:53PM +0530, Vidya Sagar wrote:
> > > On 11/22/2019 6:55 PM, Thierry Reding wrote:
> > > > On Fri, Nov 22, 2019 at 04:15:05PM +0530, Vidya Sagar wrote:
> > > > > Add endpoint mode support for PCIe C5 controller in P2972-0000 platform
> > > > > with information about supplies, PHY, PERST GPIO and GPIO that controls
> > > > > PCIe reference clock coming from the host system.
> > > > > 
> > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > > > > ---
> > > > >    .../boot/dts/nvidia/tegra194-p2972-0000.dts   | 29 +++++++++++++++++++
> > > > >    1 file changed, 29 insertions(+)
> > > > > 
> > > > > diff --git a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
> > > > > index 7eb64b816e08..58c3a9677bc8 100644
> > > > > --- a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
> > > > > +++ b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
> > > > > @@ -43,6 +43,19 @@
> > > > >    		gpio@c2f0000 {
> > > > >    			status = "okay";
> > > > > +			/*
> > > > > +			 * Change the below node's status to 'okay' when
> > > > > +			 * PCIe C5 controller is enabled to operate in endpoint
> > > > > +			 * to allow REFCLK from the host system to flow into
> > > > > +			 * the controller.
> > > > > +			 */
> > > > > +			pex-refclk-sel-high {
> > > > > +				gpio-hog;
> > > > > +				output-high;
> > > > > +				gpios = <TEGRA194_AON_GPIO(AA, 5) 0>;
> > > > > +				label = "pex_refclk_sel_high";
> > > > > +				status = "disabled";
> > > > > +			};
> > > > 
> > > > Why don't we put this into the PCIe controller's node as a reference to
> > > > that GPIO? Seems like the controller would know exactly when this pin
> > > > needs to go high or low, so why does it have to be a hog?
> > > > 
> > > > Thierry
> > > Are you saying something like 'nvidia,enable-refclk-in'?
> > > I was thinking, since this is like a board level configuration specific to Jetson-Xavier,
> > > it would suffice to just hog it according to the mode of operation of PCIe controller.
> > > But, I see one advantage of referencing it in the PCIe node (so that the driver can configure
> > > it as and when needed) is that one has to be careful just to enable either PCIe RP or EP
> > > node and not worry about other thing (like this).
> > > Let me know if I got this right.
> > 
> > Yeah, that's exactly why I think referencing this from the controller
> > and controlling it in the driver is preferable.
> > 
> > If this is some sort of select signal I think it makes sense to name it
> > "nvidia,refclk-select-gpios" or something. Does this appear in the
> > schematic somewhere? Or does the IP have a name for this? Those are
> > usually good places to look for inspiration on the name because it's
> > what hardware designers will be familiar with and they are technically
> > the ones who should write the DT, even if that's rarely the case.
> Schematic has "PEX_REFCLK_SEL" name.
> I would go with 'nvidia,refclk-select-gpios' and make the change.

It might be worth checking the interface definition of the IP if you
have access to that, since it may be using a different name from the
one that we have in the schematics.

Also, given that other instantiations don't have this, I'm beginning
to wonder if this is perhaps somehow specific to how this is used in
this particular board design. If it is, then I think the nvidia,
prefix would be appropriate. But if this is something that is part of
the IP interface then we can probably drop the prefix since it would
be applicable to non-NVIDIA instantiations as well.

Thierry

> 
> - Vidya Sagar
> > 
> > Thierry
> > 
> > > 
> > > - Vidya Sagar
> > > 
> > > > 
> > > > >    		};
> > > > >    		pwm@c340000 {
> > > > > @@ -144,6 +157,22 @@
> > > > >    			    "p2u-5", "p2u-6", "p2u-7";
> > > > >    	};
> > > > > +	pcie_ep@141a0000 {
> > > > > +		status = "disabled";
> > > > > +
> > > > > +		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";
> > > > > +	};
> > > > > +
> > > > >    	fan: fan {
> > > > >    		compatible = "pwm-fan";
> > > > >    		pwms = <&pwm4 0 45334>;
> > > > > -- 
> > > > > 2.17.1
> > > > > 
> > > 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]