All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function
@ 2017-05-12 19:58 Tim Harvey
  2017-05-12 20:25 ` Fabio Estevam
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Tim Harvey @ 2017-05-12 19:58 UTC (permalink / raw)
  To: u-boot

There is no dedicated reset signal wired up for the MX6QDL thus if the
bootloader enables the link we need some special handling to get the core
back into a state where it is safe to touch it for configuration.

While there has been some special handling in the Linux kernel to do this,
it was removed in 4.11 thus we need to do it properly in the bootloader
and therefore without this if you enable PCI in the bootloader you will hang
while booting the 4.11 kernel.

This puts the PCIe controller back into a safe state for the kernel driver
before launching the kernel.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 arch/arm/imx-common/cpu.c |  3 +++
 drivers/pci/pcie_imx.c    | 38 ++++++++++++++++++++++++++++++++++++++
 include/pci.h             |  4 ++++
 3 files changed, 45 insertions(+)

diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c
index 40fe813..74bdd24 100644
--- a/arch/arm/imx-common/cpu.c
+++ b/arch/arm/imx-common/cpu.c
@@ -275,6 +275,9 @@ u32 get_ahb_clk(void)
 
 void arch_preboot_os(void)
 {
+#if defined(CONFIG_PCIE_IMX)
+	imx_pcie_remove();
+#endif
 #if defined(CONFIG_CMD_SATA)
 	sata_stop();
 #if defined(CONFIG_MX6)
diff --git a/drivers/pci/pcie_imx.c b/drivers/pci/pcie_imx.c
index 732d59d..eab0a2b 100644
--- a/drivers/pci/pcie_imx.c
+++ b/drivers/pci/pcie_imx.c
@@ -42,6 +42,9 @@
 
 /* PCIe Port Logic registers (memory-mapped) */
 #define PL_OFFSET 0x700
+#define PCIE_PL_PFLR (PL_OFFSET + 0x08)
+#define PCIE_PL_PFLR_LINK_STATE_MASK		(0x3f << 16)
+#define PCIE_PL_PFLR_FORCE_LINK			(1 << 15)
 #define PCIE_PHY_DEBUG_R0 (PL_OFFSET + 0x28)
 #define PCIE_PHY_DEBUG_R1 (PL_OFFSET + 0x2c)
 #define PCIE_PHY_DEBUG_R1_LINK_UP		(1 << 4)
@@ -445,6 +448,36 @@ static int imx6_pcie_assert_core_reset(void)
 	/* Power up PCIe PHY */
 	setbits_le32(&gpc_regs->cntr, PCIE_PHY_PUP_REQ);
 #else
+	/*
+	 * If the bootloader already enabled the link we need some special
+	 * handling to get the core back into a state where it is safe to
+	 * touch it for configuration.  As there is no dedicated reset signal
+	 * wired up for MX6QDL, we need to manually force LTSSM into "detect"
+	 * state before completely disabling LTSSM, which is a prerequisite
+	 * for core configuration.
+	 *
+	 * If both LTSSM_ENABLE and REF_SSP_ENABLE are active we have a strong
+	 * indication that the bootloader activated the link.
+	 */
+	if (is_mx6dq()) {
+		u32 val, gpr1, gpr12;
+
+		gpr1 = readl(&iomuxc_regs->gpr[1]);
+		gpr12 = readl(&iomuxc_regs->gpr[12]);
+		if ((gpr1 & IOMUXC_GPR1_PCIE_REF_CLK_EN) &&
+		    (gpr12 & IOMUXC_GPR12_PCIE_CTL_2)) {
+			val = readl(MX6_DBI_ADDR + PCIE_PL_PFLR);
+			val &= ~PCIE_PL_PFLR_LINK_STATE_MASK;
+			val |= PCIE_PL_PFLR_FORCE_LINK;
+
+			imx_pcie_fix_dabt_handler(true);
+			writel(val, MX6_DBI_ADDR + PCIE_PL_PFLR);
+			imx_pcie_fix_dabt_handler(false);
+
+			gpr12 &= ~IOMUXC_GPR12_PCIE_CTL_2;
+			writel(val, &iomuxc_regs->gpr[12]);
+		}
+	}
 	setbits_le32(&iomuxc_regs->gpr[1], IOMUXC_GPR1_TEST_POWERDOWN);
 	clrbits_le32(&iomuxc_regs->gpr[1], IOMUXC_GPR1_REF_SSP_EN);
 #endif
@@ -652,6 +685,11 @@ void imx_pcie_init(void)
 	}
 }
 
+void imx_pcie_remove(void)
+{
+	imx6_pcie_assert_core_reset();
+}
+
 /* Probe function. */
 void pci_init_board(void)
 {
diff --git a/include/pci.h b/include/pci.h
index d3c955e..c8ef997 100644
--- a/include/pci.h
+++ b/include/pci.h
@@ -754,6 +754,10 @@ int pci_last_busno(void);
 extern void pci_mpc85xx_init (struct pci_controller *hose);
 #endif
 
+#ifdef CONFIG_PCIE_IMX
+extern void imx_pcie_remove(void);
+#endif
+
 #if !defined(CONFIG_DM_PCI) || defined(CONFIG_DM_PCI_COMPAT)
 /**
  * pci_write_bar32() - Write the address of a BAR including control bits
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function
  2017-05-12 19:58 [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function Tim Harvey
@ 2017-05-12 20:25 ` Fabio Estevam
  2017-05-14  8:10   ` Peter Senna Tschudin
  2017-05-18  9:12 ` Stefano Babic
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 15+ messages in thread
From: Fabio Estevam @ 2017-05-12 20:25 UTC (permalink / raw)
  To: u-boot

Hi Tim,

On Fri, May 12, 2017 at 4:58 PM, Tim Harvey <tharvey@gateworks.com> wrote:
> There is no dedicated reset signal wired up for the MX6QDL thus if the
> bootloader enables the link we need some special handling to get the core
> back into a state where it is safe to touch it for configuration.
>
> While there has been some special handling in the Linux kernel to do this,
> it was removed in 4.11 thus we need to do it properly in the bootloader
> and therefore without this if you enable PCI in the bootloader you will hang
> while booting the 4.11 kernel.
>
> This puts the PCIe controller back into a safe state for the kernel driver
> before launching the kernel.
>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>

This looks good:

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>

Added Peter on Cc in case he could send his Tested-by tag.

> ---
>  arch/arm/imx-common/cpu.c |  3 +++
>  drivers/pci/pcie_imx.c    | 38 ++++++++++++++++++++++++++++++++++++++
>  include/pci.h             |  4 ++++
>  3 files changed, 45 insertions(+)
>
> diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c
> index 40fe813..74bdd24 100644
> --- a/arch/arm/imx-common/cpu.c
> +++ b/arch/arm/imx-common/cpu.c
> @@ -275,6 +275,9 @@ u32 get_ahb_clk(void)
>
>  void arch_preboot_os(void)
>  {
> +#if defined(CONFIG_PCIE_IMX)
> +       imx_pcie_remove();
> +#endif
>  #if defined(CONFIG_CMD_SATA)
>         sata_stop();
>  #if defined(CONFIG_MX6)
> diff --git a/drivers/pci/pcie_imx.c b/drivers/pci/pcie_imx.c
> index 732d59d..eab0a2b 100644
> --- a/drivers/pci/pcie_imx.c
> +++ b/drivers/pci/pcie_imx.c
> @@ -42,6 +42,9 @@
>
>  /* PCIe Port Logic registers (memory-mapped) */
>  #define PL_OFFSET 0x700
> +#define PCIE_PL_PFLR (PL_OFFSET + 0x08)
> +#define PCIE_PL_PFLR_LINK_STATE_MASK           (0x3f << 16)
> +#define PCIE_PL_PFLR_FORCE_LINK                        (1 << 15)
>  #define PCIE_PHY_DEBUG_R0 (PL_OFFSET + 0x28)
>  #define PCIE_PHY_DEBUG_R1 (PL_OFFSET + 0x2c)
>  #define PCIE_PHY_DEBUG_R1_LINK_UP              (1 << 4)
> @@ -445,6 +448,36 @@ static int imx6_pcie_assert_core_reset(void)
>         /* Power up PCIe PHY */
>         setbits_le32(&gpc_regs->cntr, PCIE_PHY_PUP_REQ);
>  #else
> +       /*
> +        * If the bootloader already enabled the link we need some special
> +        * handling to get the core back into a state where it is safe to
> +        * touch it for configuration.  As there is no dedicated reset signal
> +        * wired up for MX6QDL, we need to manually force LTSSM into "detect"
> +        * state before completely disabling LTSSM, which is a prerequisite
> +        * for core configuration.
> +        *
> +        * If both LTSSM_ENABLE and REF_SSP_ENABLE are active we have a strong
> +        * indication that the bootloader activated the link.
> +        */
> +       if (is_mx6dq()) {
> +               u32 val, gpr1, gpr12;
> +
> +               gpr1 = readl(&iomuxc_regs->gpr[1]);
> +               gpr12 = readl(&iomuxc_regs->gpr[12]);
> +               if ((gpr1 & IOMUXC_GPR1_PCIE_REF_CLK_EN) &&
> +                   (gpr12 & IOMUXC_GPR12_PCIE_CTL_2)) {
> +                       val = readl(MX6_DBI_ADDR + PCIE_PL_PFLR);
> +                       val &= ~PCIE_PL_PFLR_LINK_STATE_MASK;
> +                       val |= PCIE_PL_PFLR_FORCE_LINK;
> +
> +                       imx_pcie_fix_dabt_handler(true);
> +                       writel(val, MX6_DBI_ADDR + PCIE_PL_PFLR);
> +                       imx_pcie_fix_dabt_handler(false);
> +
> +                       gpr12 &= ~IOMUXC_GPR12_PCIE_CTL_2;
> +                       writel(val, &iomuxc_regs->gpr[12]);
> +               }
> +       }
>         setbits_le32(&iomuxc_regs->gpr[1], IOMUXC_GPR1_TEST_POWERDOWN);
>         clrbits_le32(&iomuxc_regs->gpr[1], IOMUXC_GPR1_REF_SSP_EN);
>  #endif
> @@ -652,6 +685,11 @@ void imx_pcie_init(void)
>         }
>  }
>
> +void imx_pcie_remove(void)
> +{
> +       imx6_pcie_assert_core_reset();
> +}
> +
>  /* Probe function. */
>  void pci_init_board(void)
>  {
> diff --git a/include/pci.h b/include/pci.h
> index d3c955e..c8ef997 100644
> --- a/include/pci.h
> +++ b/include/pci.h
> @@ -754,6 +754,10 @@ int pci_last_busno(void);
>  extern void pci_mpc85xx_init (struct pci_controller *hose);
>  #endif
>
> +#ifdef CONFIG_PCIE_IMX
> +extern void imx_pcie_remove(void);
> +#endif
> +
>  #if !defined(CONFIG_DM_PCI) || defined(CONFIG_DM_PCI_COMPAT)
>  /**
>   * pci_write_bar32() - Write the address of a BAR including control bits
> --
> 2.7.4
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function
  2017-05-12 20:25 ` Fabio Estevam
@ 2017-05-14  8:10   ` Peter Senna Tschudin
  0 siblings, 0 replies; 15+ messages in thread
From: Peter Senna Tschudin @ 2017-05-14  8:10 UTC (permalink / raw)
  To: u-boot

On Fri, May 12, 2017 at 05:25:22PM -0300, Fabio Estevam wrote:
> Hi Tim,
> 
> On Fri, May 12, 2017 at 4:58 PM, Tim Harvey <tharvey@gateworks.com> wrote:
> > There is no dedicated reset signal wired up for the MX6QDL thus if the
> > bootloader enables the link we need some special handling to get the core
> > back into a state where it is safe to touch it for configuration.
> >
> > While there has been some special handling in the Linux kernel to do this,
> > it was removed in 4.11 thus we need to do it properly in the bootloader
> > and therefore without this if you enable PCI in the bootloader you will hang
> > while booting the 4.11 kernel.
> >
> > This puts the PCIe controller back into a safe state for the kernel driver
> > before launching the kernel.
> >
> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> 
> This looks good:
> 
> Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
> 
> Added Peter on Cc in case he could send his Tested-by tag.

I confirm this patch solves my issues. Thank you Tim!

Tested-by: Peter Senna Tschudin <peter.senna@collabora.com>

> 
> > ---
> >  arch/arm/imx-common/cpu.c |  3 +++
> >  drivers/pci/pcie_imx.c    | 38 ++++++++++++++++++++++++++++++++++++++
> >  include/pci.h             |  4 ++++
> >  3 files changed, 45 insertions(+)
> >
> > diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c
> > index 40fe813..74bdd24 100644
> > --- a/arch/arm/imx-common/cpu.c
> > +++ b/arch/arm/imx-common/cpu.c
> > @@ -275,6 +275,9 @@ u32 get_ahb_clk(void)
> >
> >  void arch_preboot_os(void)
> >  {
> > +#if defined(CONFIG_PCIE_IMX)
> > +       imx_pcie_remove();
> > +#endif
> >  #if defined(CONFIG_CMD_SATA)
> >         sata_stop();
> >  #if defined(CONFIG_MX6)
> > diff --git a/drivers/pci/pcie_imx.c b/drivers/pci/pcie_imx.c
> > index 732d59d..eab0a2b 100644
> > --- a/drivers/pci/pcie_imx.c
> > +++ b/drivers/pci/pcie_imx.c
> > @@ -42,6 +42,9 @@
> >
> >  /* PCIe Port Logic registers (memory-mapped) */
> >  #define PL_OFFSET 0x700
> > +#define PCIE_PL_PFLR (PL_OFFSET + 0x08)
> > +#define PCIE_PL_PFLR_LINK_STATE_MASK           (0x3f << 16)
> > +#define PCIE_PL_PFLR_FORCE_LINK                        (1 << 15)
> >  #define PCIE_PHY_DEBUG_R0 (PL_OFFSET + 0x28)
> >  #define PCIE_PHY_DEBUG_R1 (PL_OFFSET + 0x2c)
> >  #define PCIE_PHY_DEBUG_R1_LINK_UP              (1 << 4)
> > @@ -445,6 +448,36 @@ static int imx6_pcie_assert_core_reset(void)
> >         /* Power up PCIe PHY */
> >         setbits_le32(&gpc_regs->cntr, PCIE_PHY_PUP_REQ);
> >  #else
> > +       /*
> > +        * If the bootloader already enabled the link we need some special
> > +        * handling to get the core back into a state where it is safe to
> > +        * touch it for configuration.  As there is no dedicated reset signal
> > +        * wired up for MX6QDL, we need to manually force LTSSM into "detect"
> > +        * state before completely disabling LTSSM, which is a prerequisite
> > +        * for core configuration.
> > +        *
> > +        * If both LTSSM_ENABLE and REF_SSP_ENABLE are active we have a strong
> > +        * indication that the bootloader activated the link.
> > +        */
> > +       if (is_mx6dq()) {
> > +               u32 val, gpr1, gpr12;
> > +
> > +               gpr1 = readl(&iomuxc_regs->gpr[1]);
> > +               gpr12 = readl(&iomuxc_regs->gpr[12]);
> > +               if ((gpr1 & IOMUXC_GPR1_PCIE_REF_CLK_EN) &&
> > +                   (gpr12 & IOMUXC_GPR12_PCIE_CTL_2)) {
> > +                       val = readl(MX6_DBI_ADDR + PCIE_PL_PFLR);
> > +                       val &= ~PCIE_PL_PFLR_LINK_STATE_MASK;
> > +                       val |= PCIE_PL_PFLR_FORCE_LINK;
> > +
> > +                       imx_pcie_fix_dabt_handler(true);
> > +                       writel(val, MX6_DBI_ADDR + PCIE_PL_PFLR);
> > +                       imx_pcie_fix_dabt_handler(false);
> > +
> > +                       gpr12 &= ~IOMUXC_GPR12_PCIE_CTL_2;
> > +                       writel(val, &iomuxc_regs->gpr[12]);
> > +               }
> > +       }
> >         setbits_le32(&iomuxc_regs->gpr[1], IOMUXC_GPR1_TEST_POWERDOWN);
> >         clrbits_le32(&iomuxc_regs->gpr[1], IOMUXC_GPR1_REF_SSP_EN);
> >  #endif
> > @@ -652,6 +685,11 @@ void imx_pcie_init(void)
> >         }
> >  }
> >
> > +void imx_pcie_remove(void)
> > +{
> > +       imx6_pcie_assert_core_reset();
> > +}
> > +
> >  /* Probe function. */
> >  void pci_init_board(void)
> >  {
> > diff --git a/include/pci.h b/include/pci.h
> > index d3c955e..c8ef997 100644
> > --- a/include/pci.h
> > +++ b/include/pci.h
> > @@ -754,6 +754,10 @@ int pci_last_busno(void);
> >  extern void pci_mpc85xx_init (struct pci_controller *hose);
> >  #endif
> >
> > +#ifdef CONFIG_PCIE_IMX
> > +extern void imx_pcie_remove(void);
> > +#endif
> > +
> >  #if !defined(CONFIG_DM_PCI) || defined(CONFIG_DM_PCI_COMPAT)
> >  /**
> >   * pci_write_bar32() - Write the address of a BAR including control bits
> > --
> > 2.7.4
> >

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function
  2017-05-12 19:58 [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function Tim Harvey
  2017-05-12 20:25 ` Fabio Estevam
@ 2017-05-18  9:12 ` Stefano Babic
  2017-05-18 14:22   ` Tim Harvey
  2017-06-26 14:07 ` [U-Boot] " Soeren Moch
  2017-09-22 14:00 ` [U-Boot] [PATCH] " David Müller (ELSOFT AG)
  3 siblings, 1 reply; 15+ messages in thread
From: Stefano Babic @ 2017-05-18  9:12 UTC (permalink / raw)
  To: u-boot

On 12/05/2017 21:58, Tim Harvey wrote:
> There is no dedicated reset signal wired up for the MX6QDL thus if the
> bootloader enables the link we need some special handling to get the core
> back into a state where it is safe to touch it for configuration.
> 
> While there has been some special handling in the Linux kernel to do this,
> it was removed in 4.11 thus we need to do it properly in the bootloader
> and therefore without this if you enable PCI in the bootloader you will hang
> while booting the 4.11 kernel.
> 
> This puts the PCIe controller back into a safe state for the kernel driver
> before launching the kernel.
> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
>  arch/arm/imx-common/cpu.c |  3 +++
>  drivers/pci/pcie_imx.c    | 38 ++++++++++++++++++++++++++++++++++++++
>  include/pci.h             |  4 ++++
>  3 files changed, 45 insertions(+)
> 
> diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c
> index 40fe813..74bdd24 100644
> --- a/arch/arm/imx-common/cpu.c
> +++ b/arch/arm/imx-common/cpu.c
> @@ -275,6 +275,9 @@ u32 get_ahb_clk(void)
>  
>  void arch_preboot_os(void)
>  {
> +#if defined(CONFIG_PCIE_IMX)
> +	imx_pcie_remove();
> +#endif
>  #if defined(CONFIG_CMD_SATA)
>  	sata_stop();
>  #if defined(CONFIG_MX6)
> diff --git a/drivers/pci/pcie_imx.c b/drivers/pci/pcie_imx.c
> index 732d59d..eab0a2b 100644
> --- a/drivers/pci/pcie_imx.c
> +++ b/drivers/pci/pcie_imx.c
> @@ -42,6 +42,9 @@
>  
>  /* PCIe Port Logic registers (memory-mapped) */
>  #define PL_OFFSET 0x700
> +#define PCIE_PL_PFLR (PL_OFFSET + 0x08)
> +#define PCIE_PL_PFLR_LINK_STATE_MASK		(0x3f << 16)
> +#define PCIE_PL_PFLR_FORCE_LINK			(1 << 15)
>  #define PCIE_PHY_DEBUG_R0 (PL_OFFSET + 0x28)
>  #define PCIE_PHY_DEBUG_R1 (PL_OFFSET + 0x2c)
>  #define PCIE_PHY_DEBUG_R1_LINK_UP		(1 << 4)
> @@ -445,6 +448,36 @@ static int imx6_pcie_assert_core_reset(void)
>  	/* Power up PCIe PHY */
>  	setbits_le32(&gpc_regs->cntr, PCIE_PHY_PUP_REQ);
>  #else
> +	/*
> +	 * If the bootloader already enabled the link we need some special
> +	 * handling to get the core back into a state where it is safe to
> +	 * touch it for configuration.  As there is no dedicated reset signal
> +	 * wired up for MX6QDL, we need to manually force LTSSM into "detect"
> +	 * state before completely disabling LTSSM, which is a prerequisite
> +	 * for core configuration.
> +	 *
> +	 * If both LTSSM_ENABLE and REF_SSP_ENABLE are active we have a strong
> +	 * indication that the bootloader activated the link.
> +	 */
> +	if (is_mx6dq()) {
> +		u32 val, gpr1, gpr12;
> +
> +		gpr1 = readl(&iomuxc_regs->gpr[1]);
> +		gpr12 = readl(&iomuxc_regs->gpr[12]);
> +		if ((gpr1 & IOMUXC_GPR1_PCIE_REF_CLK_EN) &&
> +		    (gpr12 & IOMUXC_GPR12_PCIE_CTL_2)) {
> +			val = readl(MX6_DBI_ADDR + PCIE_PL_PFLR);
> +			val &= ~PCIE_PL_PFLR_LINK_STATE_MASK;
> +			val |= PCIE_PL_PFLR_FORCE_LINK;
> +
> +			imx_pcie_fix_dabt_handler(true);
> +			writel(val, MX6_DBI_ADDR + PCIE_PL_PFLR);
> +			imx_pcie_fix_dabt_handler(false);
> +
> +			gpr12 &= ~IOMUXC_GPR12_PCIE_CTL_2;
> +			writel(val, &iomuxc_regs->gpr[12]);
> +		}
> +	}
>  	setbits_le32(&iomuxc_regs->gpr[1], IOMUXC_GPR1_TEST_POWERDOWN);
>  	clrbits_le32(&iomuxc_regs->gpr[1], IOMUXC_GPR1_REF_SSP_EN);
>  #endif
> @@ -652,6 +685,11 @@ void imx_pcie_init(void)
>  	}
>  }
>  
> +void imx_pcie_remove(void)
> +{
> +	imx6_pcie_assert_core_reset();
> +}
> +
>  /* Probe function. */
>  void pci_init_board(void)
>  {
> diff --git a/include/pci.h b/include/pci.h
> index d3c955e..c8ef997 100644
> --- a/include/pci.h
> +++ b/include/pci.h
> @@ -754,6 +754,10 @@ int pci_last_busno(void);
>  extern void pci_mpc85xx_init (struct pci_controller *hose);
>  #endif
>  
> +#ifdef CONFIG_PCIE_IMX
> +extern void imx_pcie_remove(void);
> +#endif
> +
>  #if !defined(CONFIG_DM_PCI) || defined(CONFIG_DM_PCI_COMPAT)
>  /**
>   * pci_write_bar32() - Write the address of a BAR including control bits
> 

Ok, I see - now the question to Jagan. Tim has not time to move to DM,
and you propose yourself as volunteer (welcome !) to do this job. Of
course, I will not let things broken if move cannot be done, but I will
prefer to wait having a proper long time fix.

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function
  2017-05-18  9:12 ` Stefano Babic
@ 2017-05-18 14:22   ` Tim Harvey
  2017-05-18 14:35     ` Stefano Babic
  0 siblings, 1 reply; 15+ messages in thread
From: Tim Harvey @ 2017-05-18 14:22 UTC (permalink / raw)
  To: u-boot

On Thu, May 18, 2017 at 2:12 AM, Stefano Babic <sbabic@denx.de> wrote:
> On 12/05/2017 21:58, Tim Harvey wrote:
>> There is no dedicated reset signal wired up for the MX6QDL thus if the
>> bootloader enables the link we need some special handling to get the core
>> back into a state where it is safe to touch it for configuration.
>>
>> While there has been some special handling in the Linux kernel to do this,
>> it was removed in 4.11 thus we need to do it properly in the bootloader
>> and therefore without this if you enable PCI in the bootloader you will hang
>> while booting the 4.11 kernel.
>>
>> This puts the PCIe controller back into a safe state for the kernel driver
>> before launching the kernel.
>>
>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>> ---
>>  arch/arm/imx-common/cpu.c |  3 +++
>>  drivers/pci/pcie_imx.c    | 38 ++++++++++++++++++++++++++++++++++++++
>>  include/pci.h             |  4 ++++
>>  3 files changed, 45 insertions(+)
>>
>> diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c
>> index 40fe813..74bdd24 100644
>> --- a/arch/arm/imx-common/cpu.c
>> +++ b/arch/arm/imx-common/cpu.c
>> @@ -275,6 +275,9 @@ u32 get_ahb_clk(void)
>>
>>  void arch_preboot_os(void)
>>  {
>> +#if defined(CONFIG_PCIE_IMX)
>> +     imx_pcie_remove();
>> +#endif
>>  #if defined(CONFIG_CMD_SATA)
>>       sata_stop();
>>  #if defined(CONFIG_MX6)
>> diff --git a/drivers/pci/pcie_imx.c b/drivers/pci/pcie_imx.c
>> index 732d59d..eab0a2b 100644
>> --- a/drivers/pci/pcie_imx.c
>> +++ b/drivers/pci/pcie_imx.c
>> @@ -42,6 +42,9 @@
>>
>>  /* PCIe Port Logic registers (memory-mapped) */
>>  #define PL_OFFSET 0x700
>> +#define PCIE_PL_PFLR (PL_OFFSET + 0x08)
>> +#define PCIE_PL_PFLR_LINK_STATE_MASK         (0x3f << 16)
>> +#define PCIE_PL_PFLR_FORCE_LINK                      (1 << 15)
>>  #define PCIE_PHY_DEBUG_R0 (PL_OFFSET + 0x28)
>>  #define PCIE_PHY_DEBUG_R1 (PL_OFFSET + 0x2c)
>>  #define PCIE_PHY_DEBUG_R1_LINK_UP            (1 << 4)
>> @@ -445,6 +448,36 @@ static int imx6_pcie_assert_core_reset(void)
>>       /* Power up PCIe PHY */
>>       setbits_le32(&gpc_regs->cntr, PCIE_PHY_PUP_REQ);
>>  #else
>> +     /*
>> +      * If the bootloader already enabled the link we need some special
>> +      * handling to get the core back into a state where it is safe to
>> +      * touch it for configuration.  As there is no dedicated reset signal
>> +      * wired up for MX6QDL, we need to manually force LTSSM into "detect"
>> +      * state before completely disabling LTSSM, which is a prerequisite
>> +      * for core configuration.
>> +      *
>> +      * If both LTSSM_ENABLE and REF_SSP_ENABLE are active we have a strong
>> +      * indication that the bootloader activated the link.
>> +      */
>> +     if (is_mx6dq()) {
>> +             u32 val, gpr1, gpr12;
>> +
>> +             gpr1 = readl(&iomuxc_regs->gpr[1]);
>> +             gpr12 = readl(&iomuxc_regs->gpr[12]);
>> +             if ((gpr1 & IOMUXC_GPR1_PCIE_REF_CLK_EN) &&
>> +                 (gpr12 & IOMUXC_GPR12_PCIE_CTL_2)) {
>> +                     val = readl(MX6_DBI_ADDR + PCIE_PL_PFLR);
>> +                     val &= ~PCIE_PL_PFLR_LINK_STATE_MASK;
>> +                     val |= PCIE_PL_PFLR_FORCE_LINK;
>> +
>> +                     imx_pcie_fix_dabt_handler(true);
>> +                     writel(val, MX6_DBI_ADDR + PCIE_PL_PFLR);
>> +                     imx_pcie_fix_dabt_handler(false);
>> +
>> +                     gpr12 &= ~IOMUXC_GPR12_PCIE_CTL_2;
>> +                     writel(val, &iomuxc_regs->gpr[12]);
>> +             }
>> +     }
>>       setbits_le32(&iomuxc_regs->gpr[1], IOMUXC_GPR1_TEST_POWERDOWN);
>>       clrbits_le32(&iomuxc_regs->gpr[1], IOMUXC_GPR1_REF_SSP_EN);
>>  #endif
>> @@ -652,6 +685,11 @@ void imx_pcie_init(void)
>>       }
>>  }
>>
>> +void imx_pcie_remove(void)
>> +{
>> +     imx6_pcie_assert_core_reset();
>> +}
>> +
>>  /* Probe function. */
>>  void pci_init_board(void)
>>  {
>> diff --git a/include/pci.h b/include/pci.h
>> index d3c955e..c8ef997 100644
>> --- a/include/pci.h
>> +++ b/include/pci.h
>> @@ -754,6 +754,10 @@ int pci_last_busno(void);
>>  extern void pci_mpc85xx_init (struct pci_controller *hose);
>>  #endif
>>
>> +#ifdef CONFIG_PCIE_IMX
>> +extern void imx_pcie_remove(void);
>> +#endif
>> +
>>  #if !defined(CONFIG_DM_PCI) || defined(CONFIG_DM_PCI_COMPAT)
>>  /**
>>   * pci_write_bar32() - Write the address of a BAR including control bits
>>
>
> Ok, I see - now the question to Jagan. Tim has not time to move to DM,
> and you propose yourself as volunteer (welcome !) to do this job. Of
> course, I will not let things broken if move cannot be done, but I will
> prefer to wait having a proper long time fix.
>
> Best regards,
> Stefano
>

Stefano,

My patch is not intrusive and it appears several people are wanting
it. Why not accept my patch which will end up getting re-written once
the PCI driver is moved to DM?

Tim

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function
  2017-05-18 14:22   ` Tim Harvey
@ 2017-05-18 14:35     ` Stefano Babic
  2017-05-31  8:09       ` Stefano Babic
  0 siblings, 1 reply; 15+ messages in thread
From: Stefano Babic @ 2017-05-18 14:35 UTC (permalink / raw)
  To: u-boot

Hi Tim,

On 18/05/2017 16:22, Tim Harvey wrote:
> On Thu, May 18, 2017 at 2:12 AM, Stefano Babic <sbabic@denx.de> wrote:
>> On 12/05/2017 21:58, Tim Harvey wrote:
>>> There is no dedicated reset signal wired up for the MX6QDL thus if the
>>> bootloader enables the link we need some special handling to get the core
>>> back into a state where it is safe to touch it for configuration.
>>>
>>> While there has been some special handling in the Linux kernel to do this,
>>> it was removed in 4.11 thus we need to do it properly in the bootloader
>>> and therefore without this if you enable PCI in the bootloader you will hang
>>> while booting the 4.11 kernel.
>>>
>>> This puts the PCIe controller back into a safe state for the kernel driver
>>> before launching the kernel.
>>>
>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>>> ---
>>>  arch/arm/imx-common/cpu.c |  3 +++
>>>  drivers/pci/pcie_imx.c    | 38 ++++++++++++++++++++++++++++++++++++++
>>>  include/pci.h             |  4 ++++
>>>  3 files changed, 45 insertions(+)
>>>
>>> diff --git a/arch/arm/imx-common/cpu.c b/arch/arm/imx-common/cpu.c
>>> index 40fe813..74bdd24 100644
>>> --- a/arch/arm/imx-common/cpu.c
>>> +++ b/arch/arm/imx-common/cpu.c
>>> @@ -275,6 +275,9 @@ u32 get_ahb_clk(void)
>>>
>>>  void arch_preboot_os(void)
>>>  {
>>> +#if defined(CONFIG_PCIE_IMX)
>>> +     imx_pcie_remove();
>>> +#endif
>>>  #if defined(CONFIG_CMD_SATA)
>>>       sata_stop();
>>>  #if defined(CONFIG_MX6)
>>> diff --git a/drivers/pci/pcie_imx.c b/drivers/pci/pcie_imx.c
>>> index 732d59d..eab0a2b 100644
>>> --- a/drivers/pci/pcie_imx.c
>>> +++ b/drivers/pci/pcie_imx.c
>>> @@ -42,6 +42,9 @@
>>>
>>>  /* PCIe Port Logic registers (memory-mapped) */
>>>  #define PL_OFFSET 0x700
>>> +#define PCIE_PL_PFLR (PL_OFFSET + 0x08)
>>> +#define PCIE_PL_PFLR_LINK_STATE_MASK         (0x3f << 16)
>>> +#define PCIE_PL_PFLR_FORCE_LINK                      (1 << 15)
>>>  #define PCIE_PHY_DEBUG_R0 (PL_OFFSET + 0x28)
>>>  #define PCIE_PHY_DEBUG_R1 (PL_OFFSET + 0x2c)
>>>  #define PCIE_PHY_DEBUG_R1_LINK_UP            (1 << 4)
>>> @@ -445,6 +448,36 @@ static int imx6_pcie_assert_core_reset(void)
>>>       /* Power up PCIe PHY */
>>>       setbits_le32(&gpc_regs->cntr, PCIE_PHY_PUP_REQ);
>>>  #else
>>> +     /*
>>> +      * If the bootloader already enabled the link we need some special
>>> +      * handling to get the core back into a state where it is safe to
>>> +      * touch it for configuration.  As there is no dedicated reset signal
>>> +      * wired up for MX6QDL, we need to manually force LTSSM into "detect"
>>> +      * state before completely disabling LTSSM, which is a prerequisite
>>> +      * for core configuration.
>>> +      *
>>> +      * If both LTSSM_ENABLE and REF_SSP_ENABLE are active we have a strong
>>> +      * indication that the bootloader activated the link.
>>> +      */
>>> +     if (is_mx6dq()) {
>>> +             u32 val, gpr1, gpr12;
>>> +
>>> +             gpr1 = readl(&iomuxc_regs->gpr[1]);
>>> +             gpr12 = readl(&iomuxc_regs->gpr[12]);
>>> +             if ((gpr1 & IOMUXC_GPR1_PCIE_REF_CLK_EN) &&
>>> +                 (gpr12 & IOMUXC_GPR12_PCIE_CTL_2)) {
>>> +                     val = readl(MX6_DBI_ADDR + PCIE_PL_PFLR);
>>> +                     val &= ~PCIE_PL_PFLR_LINK_STATE_MASK;
>>> +                     val |= PCIE_PL_PFLR_FORCE_LINK;
>>> +
>>> +                     imx_pcie_fix_dabt_handler(true);
>>> +                     writel(val, MX6_DBI_ADDR + PCIE_PL_PFLR);
>>> +                     imx_pcie_fix_dabt_handler(false);
>>> +
>>> +                     gpr12 &= ~IOMUXC_GPR12_PCIE_CTL_2;
>>> +                     writel(val, &iomuxc_regs->gpr[12]);
>>> +             }
>>> +     }
>>>       setbits_le32(&iomuxc_regs->gpr[1], IOMUXC_GPR1_TEST_POWERDOWN);
>>>       clrbits_le32(&iomuxc_regs->gpr[1], IOMUXC_GPR1_REF_SSP_EN);
>>>  #endif
>>> @@ -652,6 +685,11 @@ void imx_pcie_init(void)
>>>       }
>>>  }
>>>
>>> +void imx_pcie_remove(void)
>>> +{
>>> +     imx6_pcie_assert_core_reset();
>>> +}
>>> +
>>>  /* Probe function. */
>>>  void pci_init_board(void)
>>>  {
>>> diff --git a/include/pci.h b/include/pci.h
>>> index d3c955e..c8ef997 100644
>>> --- a/include/pci.h
>>> +++ b/include/pci.h
>>> @@ -754,6 +754,10 @@ int pci_last_busno(void);
>>>  extern void pci_mpc85xx_init (struct pci_controller *hose);
>>>  #endif
>>>
>>> +#ifdef CONFIG_PCIE_IMX
>>> +extern void imx_pcie_remove(void);
>>> +#endif
>>> +
>>>  #if !defined(CONFIG_DM_PCI) || defined(CONFIG_DM_PCI_COMPAT)
>>>  /**
>>>   * pci_write_bar32() - Write the address of a BAR including control bits
>>>
>>
>> Ok, I see - now the question to Jagan. Tim has not time to move to DM,
>> and you propose yourself as volunteer (welcome !) to do this job. Of
>> course, I will not let things broken if move cannot be done, but I will
>> prefer to wait having a proper long time fix.
>>
>> Best regards,
>> Stefano
>>
> 
> Stefano,
> 
> My patch is not intrusive and it appears several people are wanting
> it. Why not accept my patch which will end up getting re-written once
> the PCI driver is moved to DM?

Well, most drivers are already moved to DM and there is a clear path to
go on in this direction. It is not strange or something special with PCI
driver. I am just asking if there is a volunteer to take over the job (I
understand that we are all quite busy, but Jagan offers himself as
volunteer). I fully agree that it is bad to let it broken, and I will
merge this as it is if there will not a patch in time for release.

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function
  2017-05-18 14:35     ` Stefano Babic
@ 2017-05-31  8:09       ` Stefano Babic
  0 siblings, 0 replies; 15+ messages in thread
From: Stefano Babic @ 2017-05-31  8:09 UTC (permalink / raw)
  To: u-boot

Hi Tim,

On 18/05/2017 16:35, Stefano Babic wrote:
>> Stefano,
>>
>> My patch is not intrusive and it appears several people are wanting
>> it. Why not accept my patch which will end up getting re-written once
>> the PCI driver is moved to DM?
> 
> Well, most drivers are already moved to DM and there is a clear path to
> go on in this direction. It is not strange or something special with PCI
> driver. I am just asking if there is a volunteer to take over the job (I
> understand that we are all quite busy, but Jagan offers himself as
> volunteer). I fully agree that it is bad to let it broken, and I will
> merge this as it is if there will not a patch in time for release.

As promised: I merge it and moving to DM can be done in a follow-up
patch when someone has time for it. In the meantime, this fix a grave issue.

Applied to -master, thanks !

Best regards,
Stefano Babic



-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] drivers: pci: imx: add imx_pcie_remove function
  2017-05-12 19:58 [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function Tim Harvey
  2017-05-12 20:25 ` Fabio Estevam
  2017-05-18  9:12 ` Stefano Babic
@ 2017-06-26 14:07 ` Soeren Moch
  2017-06-27  7:07   ` Stefano Babic
  2017-09-22 14:00 ` [U-Boot] [PATCH] " David Müller (ELSOFT AG)
  3 siblings, 1 reply; 15+ messages in thread
From: Soeren Moch @ 2017-06-26 14:07 UTC (permalink / raw)
  To: u-boot



On 12.05.2017 21:58, Tim Harvey wrote:
> There is no dedicated reset signal wired up for the MX6QDL thus if the
> bootloader enables the link we need some special handling to get the core
> back into a state where it is safe to touch it for configuration.
>
> While there has been some special handling in the Linux kernel to do this,
> it was removed in 4.11 thus we need to do it properly in the bootloader
> and therefore without this if you enable PCI in the bootloader you will hang
> while booting the 4.11 kernel.
>
> This puts the PCIe controller back into a safe state for the kernel driver
> before launching the kernel.
>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
> Tested-by: Peter Senna Tschudin <peter.senna@collabora.com>

Tested-by: Soeren Moch <smoch@web.de>



Stefano,

can we get this fix merged for v2017.07!?

Thanks,
Soeren

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] drivers: pci: imx: add imx_pcie_remove function
  2017-06-26 14:07 ` [U-Boot] " Soeren Moch
@ 2017-06-27  7:07   ` Stefano Babic
  0 siblings, 0 replies; 15+ messages in thread
From: Stefano Babic @ 2017-06-27  7:07 UTC (permalink / raw)
  To: u-boot

On 26/06/2017 16:07, Soeren Moch wrote:
> 
> 
> On 12.05.2017 21:58, Tim Harvey wrote:
>> There is no dedicated reset signal wired up for the MX6QDL thus if the
>> bootloader enables the link we need some special handling to get the core
>> back into a state where it is safe to touch it for configuration.
>>
>> While there has been some special handling in the Linux kernel to do this,
>> it was removed in 4.11 thus we need to do it properly in the bootloader
>> and therefore without this if you enable PCI in the bootloader you will hang
>> while booting the 4.11 kernel.
>>
>> This puts the PCIe controller back into a safe state for the kernel driver
>> before launching the kernel.
>>
>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>> Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
>> Tested-by: Peter Senna Tschudin <peter.senna@collabora.com>
> 
> Tested-by: Soeren Moch <smoch@web.de>
> 
> 
> 
> Stefano,
> 
> can we get this fix merged for v2017.07!?

Yes - this is merged in u-boot-imx, I am checking now which pending
patches are bug fixes that can flow in, and I send later my PR to Tom.

Regards,
Stefano


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function
  2017-05-12 19:58 [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function Tim Harvey
                   ` (2 preceding siblings ...)
  2017-06-26 14:07 ` [U-Boot] " Soeren Moch
@ 2017-09-22 14:00 ` David Müller (ELSOFT AG)
  2017-09-22 18:28   ` Marek Vasut
                     ` (2 more replies)
  3 siblings, 3 replies; 15+ messages in thread
From: David Müller (ELSOFT AG) @ 2017-09-22 14:00 UTC (permalink / raw)
  To: u-boot

Hello

Does the code below really work?

On my custom MX6Q board, the code hangs on the read of the
"PCIE_PL_PFLR". Please note that this code sequence is not entered the
first time after a power up; I have to execute a U-Boot reset to
actually trigger the hang. Any ideas what is going wrong?


While debugging it, I also noticed the two problems below.

Tim Harvey wrote:

> +	if (is_mx6dq()) {
> +		u32 val, gpr1, gpr12;
> +
> +		gpr1 = readl(&iomuxc_regs->gpr[1]);
> +		gpr12 = readl(&iomuxc_regs->gpr[12]);
> +		if ((gpr1 & IOMUXC_GPR1_PCIE_REF_CLK_EN) &&
> +		    (gpr12 & IOMUXC_GPR12_PCIE_CTL_2)) {

This could be 	    (gpr12 & IOMUXC_GPR12_APPS_LTSSM_ENABLE)) {

> +			val = readl(MX6_DBI_ADDR + PCIE_PL_PFLR);
> +			val &= ~PCIE_PL_PFLR_LINK_STATE_MASK;
> +			val |= PCIE_PL_PFLR_FORCE_LINK;
> +
> +			imx_pcie_fix_dabt_handler(true);
> +			writel(val, MX6_DBI_ADDR + PCIE_PL_PFLR);
> +			imx_pcie_fix_dabt_handler(false);
> +
> +			gpr12 &= ~IOMUXC_GPR12_PCIE_CTL_2;
> +			writel(val, &iomuxc_regs->gpr[12]);

I think this should be
			writel(gpr12, &iomuxc_regs->gpr[12]);

or even better
			clrbits_le32(&iomuxc_regs->gpr[12],
				 	IOMUXC_GPR12_APPS_LTSSM_ENABLE);

as in the rest of the file.

Dave

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function
  2017-09-22 14:00 ` [U-Boot] [PATCH] " David Müller (ELSOFT AG)
@ 2017-09-22 18:28   ` Marek Vasut
  2017-09-25  9:05     ` David Müller (ELSOFT AG)
  2017-09-25 16:01   ` Tim Harvey
  2017-10-05 11:47   ` Fabio Estevam
  2 siblings, 1 reply; 15+ messages in thread
From: Marek Vasut @ 2017-09-22 18:28 UTC (permalink / raw)
  To: u-boot

On 09/22/2017 04:00 PM, David Müller (ELSOFT AG) wrote:
> Hello
> 
> Does the code below really work?
> 
> On my custom MX6Q board, the code hangs on the read of the
> "PCIE_PL_PFLR". Please note that this code sequence is not entered the
> first time after a power up; I have to execute a U-Boot reset to
> actually trigger the hang. Any ideas what is going wrong?

MX6Q PCIe reset breakage strikes again ?

> While debugging it, I also noticed the two problems below.
> 
> Tim Harvey wrote:
> 
>> +	if (is_mx6dq()) {
>> +		u32 val, gpr1, gpr12;
>> +
>> +		gpr1 = readl(&iomuxc_regs->gpr[1]);
>> +		gpr12 = readl(&iomuxc_regs->gpr[12]);
>> +		if ((gpr1 & IOMUXC_GPR1_PCIE_REF_CLK_EN) &&
>> +		    (gpr12 & IOMUXC_GPR12_PCIE_CTL_2)) {
> 
> This could be 	    (gpr12 & IOMUXC_GPR12_APPS_LTSSM_ENABLE)) {
> 
>> +			val = readl(MX6_DBI_ADDR + PCIE_PL_PFLR);
>> +			val &= ~PCIE_PL_PFLR_LINK_STATE_MASK;
>> +			val |= PCIE_PL_PFLR_FORCE_LINK;
>> +
>> +			imx_pcie_fix_dabt_handler(true);
>> +			writel(val, MX6_DBI_ADDR + PCIE_PL_PFLR);
>> +			imx_pcie_fix_dabt_handler(false);
>> +
>> +			gpr12 &= ~IOMUXC_GPR12_PCIE_CTL_2;
>> +			writel(val, &iomuxc_regs->gpr[12]);
> 
> I think this should be
> 			writel(gpr12, &iomuxc_regs->gpr[12]);
> 
> or even better
> 			clrbits_le32(&iomuxc_regs->gpr[12],
> 				 	IOMUXC_GPR12_APPS_LTSSM_ENABLE);
> 
> as in the rest of the file.
> 
> Dave
> 


-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function
  2017-09-22 18:28   ` Marek Vasut
@ 2017-09-25  9:05     ` David Müller (ELSOFT AG)
  2017-09-25  9:25       ` Marek Vasut
  0 siblings, 1 reply; 15+ messages in thread
From: David Müller (ELSOFT AG) @ 2017-09-25  9:05 UTC (permalink / raw)
  To: u-boot

Marek Vasut wrote:

> On 09/22/2017 04:00 PM, David Müller (ELSOFT AG) wrote:
>> On my custom MX6Q board, the code hangs on the read of the 
>> "PCIE_PL_PFLR". Please note that this code sequence is not entered
>> the first time after a power up; I have to execute a U-Boot reset
>> to actually trigger the hang. Any ideas what is going wrong?
> 
> MX6Q PCIe reset breakage strikes again ?

I thought this piece of code was added as work-around to fix the "MX6Q
PCIe reset" problem. Or are you talking about something different?


Dave

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function
  2017-09-25  9:05     ` David Müller (ELSOFT AG)
@ 2017-09-25  9:25       ` Marek Vasut
  0 siblings, 0 replies; 15+ messages in thread
From: Marek Vasut @ 2017-09-25  9:25 UTC (permalink / raw)
  To: u-boot

On 09/25/2017 11:05 AM, David Müller (ELSOFT AG) wrote:
> Marek Vasut wrote:
> 
>> On 09/22/2017 04:00 PM, David Müller (ELSOFT AG) wrote:
>>> On my custom MX6Q board, the code hangs on the read of the 
>>> "PCIE_PL_PFLR". Please note that this code sequence is not entered
>>> the first time after a power up; I have to execute a U-Boot reset
>>> to actually trigger the hang. Any ideas what is going wrong?
>>
>> MX6Q PCIe reset breakage strikes again ?
> 
> I thought this piece of code was added as work-around to fix the "MX6Q
> PCIe reset" problem. Or are you talking about something different?

That's exactly what I'm talking about ...

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function
  2017-09-22 14:00 ` [U-Boot] [PATCH] " David Müller (ELSOFT AG)
  2017-09-22 18:28   ` Marek Vasut
@ 2017-09-25 16:01   ` Tim Harvey
  2017-10-05 11:47   ` Fabio Estevam
  2 siblings, 0 replies; 15+ messages in thread
From: Tim Harvey @ 2017-09-25 16:01 UTC (permalink / raw)
  To: u-boot

On Fri, Sep 22, 2017 at 7:00 AM, David Müller (ELSOFT AG)
<d.mueller@elsoft.ch> wrote:
> Hello
>
> Does the code below really work?

Hi Dave,

This patch does work to resolve the issue stated in the commit log
which was to fix failing to boot on a 4.11+ kernel which stems from
the fact that we have no reliable way to reset the PCIe host
controller for IMX6SDL/DQ. This code used to be in the kernel [1] but
was removed in 4.11 because it was seen as a workaround for the
bootloader failing to put the PCIe host controller back in a sane
state (something normally not needed to be done if these hardware
blocks have a reset available).

However, your right you do end up hanging on a 'soft reset' likely for
the same reason the kernel used to - the PCIe host controller is not
left in a sane state because this code isn't run in that case.

>
> On my custom MX6Q board, the code hangs on the read of the
> "PCIE_PL_PFLR". Please note that this code sequence is not entered the
> first time after a power up; I have to execute a U-Boot reset to
> actually trigger the hang. Any ideas what is going wrong?
>
>
> While debugging it, I also noticed the two problems below.
>
> Tim Harvey wrote:
>
>> +     if (is_mx6dq()) {
>> +             u32 val, gpr1, gpr12;
>> +
>> +             gpr1 = readl(&iomuxc_regs->gpr[1]);
>> +             gpr12 = readl(&iomuxc_regs->gpr[12]);
>> +             if ((gpr1 & IOMUXC_GPR1_PCIE_REF_CLK_EN) &&
>> +                 (gpr12 & IOMUXC_GPR12_PCIE_CTL_2)) {
>
> This could be       (gpr12 & IOMUXC_GPR12_APPS_LTSSM_ENABLE)) {

Currently IOMUXC_GPR12_PCIE_CTL_2 is defined as GPR12[10] likely
because that's how it was defined in an early reference manual but I
would agree it should be changed for readability - its not a bug.

>
>> +                     val = readl(MX6_DBI_ADDR + PCIE_PL_PFLR);
>> +                     val &= ~PCIE_PL_PFLR_LINK_STATE_MASK;
>> +                     val |= PCIE_PL_PFLR_FORCE_LINK;
>> +
>> +                     imx_pcie_fix_dabt_handler(true);
>> +                     writel(val, MX6_DBI_ADDR + PCIE_PL_PFLR);
>> +                     imx_pcie_fix_dabt_handler(false);
>> +
>> +                     gpr12 &= ~IOMUXC_GPR12_PCIE_CTL_2;
>> +                     writel(val, &iomuxc_regs->gpr[12]);
>
> I think this should be
>                         writel(gpr12, &iomuxc_regs->gpr[12]);
>
> or even better
>                         clrbits_le32(&iomuxc_regs->gpr[12],
>                                         IOMUXC_GPR12_APPS_LTSSM_ENABLE);

Yes this is a bug for sure, however it doesn't appear to resolve your issue.

Lucas, does barebox suffer from a hang on PCI init on chip-level reset
and if not do you know what could be causing the read of PCIE_PL_PFLR
to hang here?

Regards,

Tim

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/host/pci-imx6.c?h=v4.10#n270

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function
  2017-09-22 14:00 ` [U-Boot] [PATCH] " David Müller (ELSOFT AG)
  2017-09-22 18:28   ` Marek Vasut
  2017-09-25 16:01   ` Tim Harvey
@ 2017-10-05 11:47   ` Fabio Estevam
  2 siblings, 0 replies; 15+ messages in thread
From: Fabio Estevam @ 2017-10-05 11:47 UTC (permalink / raw)
  To: u-boot

Hi David,

On Fri, Sep 22, 2017 at 11:00 AM, David Müller (ELSOFT AG)
<d.mueller@elsoft.ch> wrote:
> Hello
>
> Does the code below really work?
>
> On my custom MX6Q board, the code hangs on the read of the
> "PCIE_PL_PFLR". Please note that this code sequence is not entered the
> first time after a power up; I have to execute a U-Boot reset to
> actually trigger the hang. Any ideas what is going wrong?

Just sent a patch that should probably fix the PCIE_PL_PFLR hang problem.

Please give it a try.

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2017-10-05 11:47 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-12 19:58 [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function Tim Harvey
2017-05-12 20:25 ` Fabio Estevam
2017-05-14  8:10   ` Peter Senna Tschudin
2017-05-18  9:12 ` Stefano Babic
2017-05-18 14:22   ` Tim Harvey
2017-05-18 14:35     ` Stefano Babic
2017-05-31  8:09       ` Stefano Babic
2017-06-26 14:07 ` [U-Boot] " Soeren Moch
2017-06-27  7:07   ` Stefano Babic
2017-09-22 14:00 ` [U-Boot] [PATCH] " David Müller (ELSOFT AG)
2017-09-22 18:28   ` Marek Vasut
2017-09-25  9:05     ` David Müller (ELSOFT AG)
2017-09-25  9:25       ` Marek Vasut
2017-09-25 16:01   ` Tim Harvey
2017-10-05 11:47   ` Fabio Estevam

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.