[U-Boot] [PATCH 0/3] fix apalis_t30 optional pcie operation

[U-Boot] [PATCH 0/3] fix apalis_t30 optional pcie operation

[Marcel Ziswiler]

This series addresses a PCIe reliability issue as observed on Apalis T30
related to a PCIe reset timing violation.

This series depends on Simon's work available at u-boot-dm/master plus
my previous series "move apalis t30/tk1, colibri t20/t30 to livetree"
and "fix apalis-tk1 pcie gigabit ethernet operation".

This series is available at http://git.toradex.com/cgit/u-boot-toradex.git/log/?h=for-next


Marcel Ziswiler (3):
  apalis_t30: describe pcie ports
  apalis_t30: fix pcie port 0 and 1 pin muxing
  apalis_t30: fix optional pcie port reset for reliable pcie operation

 arch/arm/dts/tegra30-apalis.dts                    |  3 ++
 board/toradex/apalis_t30/apalis_t30.c              | 47 ++++++++++++++++++++++
 .../toradex/apalis_t30/pinmux-config-apalis_t30.h  | 12 +++---
 include/configs/apalis_t30.h                       |  1 +
 4 files changed, 57 insertions(+), 6 deletions(-)

-- 
2.9.4

[U-Boot] [PATCH 1/3] apalis_t30: describe pcie ports

[Marcel Ziswiler]

From: Marcel Ziswiler <marcel.ziswiler@toradex.com>

Add some more comments describing the various PCIe ports available.

Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
---

 arch/arm/dts/tegra30-apalis.dts | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/dts/tegra30-apalis.dts b/arch/arm/dts/tegra30-apalis.dts
index 0b84dae..0852d8d 100644
--- a/arch/arm/dts/tegra30-apalis.dts
+++ b/arch/arm/dts/tegra30-apalis.dts
@@ -43,16 +43,19 @@
 		vddio-pex-ctl-supply = <&sys_3v3_reg>;
 		hvdd-pex-supply = <&sys_3v3_reg>;
 
+		/* Apalis Type Specific 4 Lane PCIe */
 		pci at 1,0 {
 			/* TS_DIFF1/2/3/4 left disabled */
 			nvidia,num-lanes = <4>;
 		};
 
+		/* Apalis PCIe */
 		pci at 2,0 {
 			/* PCIE1_RX/TX left disabled */
 			nvidia,num-lanes = <1>;
 		};
 
+		/* I210 Gigabit Ethernet Controller (On-module) */
 		pci at 3,0 {
 			status = "okay";
 			nvidia,num-lanes = <1>;
-- 
2.9.4

[U-Boot] [PATCH 1/3] apalis_t30: describe pcie ports

[Simon Glass]

On 8 August 2017 at 06:43, Marcel Ziswiler <marcel@ziswiler.com> wrote:
> From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
>
> Add some more comments describing the various PCIe ports available.
>
> Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> ---
>
>  arch/arm/dts/tegra30-apalis.dts | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 2/3] apalis_t30: fix pcie port 0 and 1 pin muxing

[Marcel Ziswiler]

From: Marcel Ziswiler <marcel.ziswiler@toradex.com>

Fix optional Apalis type specific 4 lane PCIe port 0 and Apalis PCIe
port 1 pin muxing.

Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
---

 board/toradex/apalis_t30/pinmux-config-apalis_t30.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/board/toradex/apalis_t30/pinmux-config-apalis_t30.h b/board/toradex/apalis_t30/pinmux-config-apalis_t30.h
index e0b00ea..6c30631 100644
--- a/board/toradex/apalis_t30/pinmux-config-apalis_t30.h
+++ b/board/toradex/apalis_t30/pinmux-config-apalis_t30.h
@@ -285,14 +285,14 @@ static struct pmux_pingrp_config tegra3_pinmux_common[] = {
 	DEFAULT_PINMUX(SPI1_CS0_N_PX6, SPI1, NORMAL, NORMAL, INPUT),
 	DEFAULT_PINMUX(SPI1_MISO_PX7, SPI1, NORMAL, NORMAL, INPUT),
 
-	DEFAULT_PINMUX(PEX_L0_PRSNT_N_PDD0, PCIE, NORMAL, NORMAL, INPUT),
-	DEFAULT_PINMUX(PEX_L0_RST_N_PDD1, PCIE, NORMAL, NORMAL, OUTPUT),
-	DEFAULT_PINMUX(PEX_L0_CLKREQ_N_PDD2, PCIE, NORMAL, NORMAL, INPUT),
+	DEFAULT_PINMUX(PEX_L0_PRSNT_N_PDD0, RSVD2, NORMAL, NORMAL, OUTPUT),
+	DEFAULT_PINMUX(PEX_L0_RST_N_PDD1, RSVD2, NORMAL, NORMAL, OUTPUT),
+	DEFAULT_PINMUX(PEX_L0_CLKREQ_N_PDD2, RSVD2, NORMAL, NORMAL, INPUT),
 	DEFAULT_PINMUX(PEX_WAKE_N_PDD3, PCIE, NORMAL, NORMAL, INPUT),
 
-	DEFAULT_PINMUX(PEX_L1_PRSNT_N_PDD4, PCIE, DOWN, TRISTATE, OUTPUT), /* NC */
-	DEFAULT_PINMUX(PEX_L1_RST_N_PDD5, PCIE, DOWN, TRISTATE, OUTPUT), /* NC */
-	DEFAULT_PINMUX(PEX_L1_CLKREQ_N_PDD6, PCIE, DOWN, TRISTATE, OUTPUT), /* NC */
+	DEFAULT_PINMUX(PEX_L1_PRSNT_N_PDD4, RSVD2, DOWN, TRISTATE, OUTPUT), /* NC */
+	DEFAULT_PINMUX(PEX_L1_RST_N_PDD5, RSVD2, DOWN, TRISTATE, OUTPUT), /* NC */
+	DEFAULT_PINMUX(PEX_L1_CLKREQ_N_PDD6, RSVD2, DOWN, TRISTATE, OUTPUT), /* NC */
 	DEFAULT_PINMUX(PEX_L2_PRSNT_N_PDD7, PCIE, NORMAL, NORMAL, INPUT),
 	DEFAULT_PINMUX(PEX_L2_RST_N_PCC6, PCIE, NORMAL, NORMAL, OUTPUT),
 	DEFAULT_PINMUX(PEX_L2_CLKREQ_N_PCC7, PCIE, NORMAL, NORMAL, INPUT),
-- 
2.9.4

[U-Boot] [PATCH 3/3] apalis_t30: fix optional pcie port reset for reliable pcie operation

[Marcel Ziswiler]

From: Marcel Ziswiler <marcel.ziswiler@toradex.com>

Allow optionally bringing up the Apalis type specific 4 lane PCIe port
as well as the PCIe switch as found on the Apalis Evaluation board. In
order to avoid violating the PCIe reset timing do this by overriding the
tegra_pcie_board_port_reset() function. Note however that both the
Apalis type specific 4 lane PCIe port as well as the regular Apalis PCIe
port are also left disabled in the device tree by default.

Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
---

 board/toradex/apalis_t30/apalis_t30.c | 47 +++++++++++++++++++++++++++++++++++
 include/configs/apalis_t30.h          |  1 +
 2 files changed, 48 insertions(+)

diff --git a/board/toradex/apalis_t30/apalis_t30.c b/board/toradex/apalis_t30/apalis_t30.c
index 827eefd..e997429 100644
--- a/board/toradex/apalis_t30/apalis_t30.c
+++ b/board/toradex/apalis_t30/apalis_t30.c
@@ -14,6 +14,7 @@
 #include <asm/io.h>
 #include <dm.h>
 #include <i2c.h>
+#include <pci_tegra.h>
 #include "../common/tdx-common.h"
 
 #include "pinmux-config-apalis_t30.h"
@@ -23,6 +24,11 @@ DECLARE_GLOBAL_DATA_PTR;
 #define PMU_I2C_ADDRESS		0x2D
 #define MAX_I2C_RETRY		3
 
+#ifdef APALIS_T30_PCIE_EVALBOARD_INIT
+#define PEX_PERST_N	TEGRA_GPIO(S, 7) /* Apalis GPIO7 */
+#define RESET_MOCI_CTRL	TEGRA_GPIO(I, 4)
+#endif /* APALIS_T30_PCIE_EVALBOARD_INIT */
+
 int arch_misc_init(void)
 {
 	if (readl(NV_PA_BASE_SRAM + NVBOOTINFOTABLE_BOOTTYPE) ==
@@ -107,6 +113,47 @@ int tegra_pcie_board_init(void)
 		return err;
 	}
 
+#ifdef APALIS_T30_PCIE_EVALBOARD_INIT
+	gpio_request(PEX_PERST_N, "PEX_PERST_N");
+	gpio_request(RESET_MOCI_CTRL, "RESET_MOCI_CTRL");
+#endif /* APALIS_T30_PCIE_EVALBOARD_INIT */
+
 	return 0;
 }
+
+void tegra_pcie_board_port_reset(void *port)
+{
+	int index = tegra_pcie_port_index_of_port(port);
+	if (index == 2) { /* I210 Gigabit Ethernet Controller (On-module) */
+		tegra_pcie_port_reset(port);
+#ifdef APALIS_T30_PCIE_EVALBOARD_INIT
+	} else if (index == 1) { /* Apalis PCIe */
+		/*
+		 * As port 0 and 1 share the same RESET_MOCI only assert it once
+		 * for both ports below to avoid loosing the previously brought
+		 * up port again.
+		 */
+	} else if (index == 0) { /* Apalis Type Specific 4 Lane PCIe */
+		/*
+		 * Reset PLX PEX 8605 PCIe Switch plus PCIe devices on Apalis
+		 * Evaluation Board
+		 */
+		gpio_direction_output(PEX_PERST_N, 0);
+		gpio_direction_output(RESET_MOCI_CTRL, 0);
+
+		/*
+		 * Must be asserted for 100 ms after power and clocks are stable
+		 */
+		mdelay(100);
+
+		gpio_set_value(PEX_PERST_N, 1);
+		/*
+		 * Err_5: PEX_REFCLK_OUTpx/nx Clock Outputs is not Guaranteed
+		 * Until 900 us After PEX_PERST# De-assertion
+		 */
+		mdelay(1);
+		gpio_set_value(RESET_MOCI_CTRL, 1);
+#endif /* APALIS_T30_PCIE_EVALBOARD_INIT */
+	}
+}
 #endif /* CONFIG_PCI_TEGRA */
diff --git a/include/configs/apalis_t30.h b/include/configs/apalis_t30.h
index daa3be0..f2a8b03 100644
--- a/include/configs/apalis_t30.h
+++ b/include/configs/apalis_t30.h
@@ -36,6 +36,7 @@
 
 /* PCI host support */
 #define CONFIG_CMD_PCI
+#undef APALIS_T30_PCIE_EVALBOARD_INIT
 
 /* PCI networking support */
 #define CONFIG_E1000_NO_NVM
-- 
2.9.4

[U-Boot] [PATCH 3/3] apalis_t30: fix optional pcie port reset for reliable pcie operation

[Stephen Warren]

On 08/08/2017 06:43 AM, Marcel Ziswiler wrote:
> From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> 
> Allow optionally bringing up the Apalis type specific 4 lane PCIe port
> as well as the PCIe switch as found on the Apalis Evaluation board. In
> order to avoid violating the PCIe reset timing do this by overriding the
> tegra_pcie_board_port_reset() function. Note however that both the
> Apalis type specific 4 lane PCIe port as well as the regular Apalis PCIe
> port are also left disabled in the device tree by default.

> diff --git a/board/toradex/apalis_t30/apalis_t30.c b/board/toradex/apalis_t30/apalis_t30.c

> +#ifdef APALIS_T30_PCIE_EVALBOARD_INIT
> +#define PEX_PERST_N	TEGRA_GPIO(S, 7) /* Apalis GPIO7 */
> +#define RESET_MOCI_CTRL	TEGRA_GPIO(I, 4)
> +#endif /* APALIS_T30_PCIE_EVALBOARD_INIT */

Shouldn't that be a CONFIG_xxx option (defined in Kconfig) rather than 
something new added to the old-style config header?

> +void tegra_pcie_board_port_reset(void *port)
> +{
> +	int index = tegra_pcie_port_index_of_port(port);
> +	if (index == 2) { /* I210 Gigabit Ethernet Controller (On-module) */
> +		tegra_pcie_port_reset(port);

I think it'd read better if the } were here rather than wrapping it onto 
the else line; a special case where ifdefs are involved.

> +#ifdef APALIS_T30_PCIE_EVALBOARD_INIT
> +	} else if (index == 1) { /* Apalis PCIe */
> +		/*
> +		 * As port 0 and 1 share the same RESET_MOCI only assert it once
> +		 * for both ports below to avoid loosing the previously brought
> +		 * up port again.
> +		 */

How do you know that port 0 gets reset first then port 1? Also, how do 
you know that the user isn't going to initialize PCIe multiple times and 
expect the HW to get reset when they do? This HW design seems fragile, 
but I suppose it's rather difficult to change already-shipped HW:-) To 
address some of the issues, does it make sense to keep state in this 
function and do the reset the first time either index 0 or 1 is reset, 
and not the second time, so this code doesn't care about the order? To 
account for re-initialization, you could perhaps do the reset every even 
(second) time the function is called for index 0 or 1, not just the first.

Aside from that, this series seems fine.

[U-Boot] [PATCH 3/3] apalis_t30: fix optional pcie port reset for reliable pcie operation

[Marcel Ziswiler]

Hi Stephen

On Tue, 2017-08-08 at 10:14 -0600, Stephen Warren wrote:
> On 08/08/2017 06:43 AM, Marcel Ziswiler wrote:
> > From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > 
> > Allow optionally bringing up the Apalis type specific 4 lane PCIe
> > port
> > as well as the PCIe switch as found on the Apalis Evaluation board.
> > In
> > order to avoid violating the PCIe reset timing do this by
> > overriding the
> > tegra_pcie_board_port_reset() function. Note however that both the
> > Apalis type specific 4 lane PCIe port as well as the regular Apalis
> > PCIe
> > port are also left disabled in the device tree by default.
> > diff --git a/board/toradex/apalis_t30/apalis_t30.c
> > b/board/toradex/apalis_t30/apalis_t30.c
> > +#ifdef APALIS_T30_PCIE_EVALBOARD_INIT
> > +#define PEX_PERST_N	TEGRA_GPIO(S, 7) /* Apalis GPIO7 */
> > +#define RESET_MOCI_CTRL	TEGRA_GPIO(I, 4)
> > +#endif /* APALIS_T30_PCIE_EVALBOARD_INIT */
> 
> Shouldn't that be a CONFIG_xxx option (defined in Kconfig) rather
> than 
> something new added to the old-style config header?

Yes, of course. Excuse my nostalgia (;-p).

> > +void tegra_pcie_board_port_reset(void *port)
> > +{
> > +	int index = tegra_pcie_port_index_of_port(port);
> > +	if (index == 2) { /* I210 Gigabit Ethernet Controller (On-
> > module) */
> > +		tegra_pcie_port_reset(port);
> 
> I think it'd read better if the } were here rather than wrapping it
> onto 
> the else line; a special case where ifdefs are involved.

Agreed.

> > +#ifdef APALIS_T30_PCIE_EVALBOARD_INIT
> > +	} else if (index == 1) { /* Apalis PCIe */
> > +		/*
> > +		 * As port 0 and 1 share the same RESET_MOCI only
> > assert it once
> > +		 * for both ports below to avoid loosing the
> > previously brought
> > +		 * up port again.
> > +		 */
> 
> How do you know that port 0 gets reset first then port 1?

I of course know from looking at resp. driver but agreed this is kind
of an internal implementation thing which may change in the future
which would break this.

> Also, how do 
> you know that the user isn't going to initialize PCIe multiple times
> and 
> expect the HW to get reset when they do?

Good question. However after having it implemented as requested below
keeping status and such it turns out there is no easy way to ever re-
initialise at least not with nowadays driver model in place (e.g. see
DM_FLAG_ACTIVATED in device_probe() of drivers/core/device.c). Or did I
miss anything?

> This HW design seems fragile,

Agreed, however it is kind of the typical case when there are certain
HW chip erratas which the board designers decide not to fix in hardware
but rather relying on a software workaround.
 
> but I suppose it's rather difficult to change already-shipped HW:-)
> To 
> address some of the issues, does it make sense to keep state in this 
> function and do the reset the first time either index 0 or 1 is
> reset, 
> and not the second time, so this code doesn't care about the order? 

Yes, I actually implemented it that way now but as mentioned above I
don't think there is any easy way to ever cause any re-enumeration.

> To 
> account for re-initialization, you could perhaps do the reset every
> even 
> (second) time the function is called for index 0 or 1, not just the
> first.

Yes, that's exactly how the upcoming v2 does it.

> Aside from that, this series seems fine.

Thanks, Stephen.

Cheers

Marcel

[U-Boot] [PATCH 3/3] apalis_t30: fix optional pcie port reset for reliable pcie operation

[Stephen Warren]

On 08/09/2017 08:53 AM, Marcel Ziswiler wrote:
> Hi Stephen
> 
> On Tue, 2017-08-08 at 10:14 -0600, Stephen Warren wrote:
>> On 08/08/2017 06:43 AM, Marcel Ziswiler wrote:
>>> From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
>>>
>>> Allow optionally bringing up the Apalis type specific 4 lane PCIe port
>>> as well as the PCIe switch as found on the Apalis Evaluation board. In
>>> order to avoid violating the PCIe reset timing do this by overriding the
>>> tegra_pcie_board_port_reset() function. Note however that both the
>>> Apalis type specific 4 lane PCIe port as well as the regular Apalis PCIe
>>> port are also left disabled in the device tree by default.
...
>> Also, how do
>> you know that the user isn't going to initialize PCIe multiple times and
>> expect the HW to get reset when they do?
> 
> Good question. However after having it implemented as requested below
> keeping status and such it turns out there is no easy way to ever re-
> initialise at least not with nowadays driver model in place (e.g. see
> DM_FLAG_ACTIVATED in device_probe() of drivers/core/device.c). Or did I
> miss anything?

I was naively assuming that "pci enum" would reset everything prior to 
re-enumeration. Perhaps it doesn't though; I didn't check the code.