| Message ID | 1494434600-24661-1-git-send-email-tharvey@gateworks.com |
|---|---|
| State | RFC |
| Delegated to: | Stefano Babic |
| Headers | show |
Hi Tim, Thanks for working on this! [Adding Peter and Jagan on Cc] On Wed, May 10, 2017 at 1:43 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 was tested on a Gateworks Ventana board. > > TODO: what is the right arch specific place to hook the remove function? Ideally the pcie_imx.c driver should be converted to driver model and then the .remove can be simply added, just like in Barebox. Take a look at the following PCI drivers that have already been converted to the driver model to use as reference: $ git grep U_BOOT_DRIVER drivers/pci/ drivers/pci/pci-uclass.c:U_BOOT_DRIVER(pci_bridge_drv) = { drivers/pci/pci-uclass.c:U_BOOT_DRIVER(pci_generic_drv) = { drivers/pci/pci_sandbox.c:U_BOOT_DRIVER(pci_sandbox) = { drivers/pci/pci_tegra.c:U_BOOT_DRIVER(pci_tegra) = { drivers/pci/pci_x86.c:U_BOOT_DRIVER(pci_x86) = { drivers/pci/pcie_dw_mvebu.c:U_BOOT_DRIVER(pcie_dw_mvebu) = { drivers/pci/pcie_layerscape.c:U_BOOT_DRIVER(pci_layerscape) = { drivers/pci/pcie_xilinx.c:U_BOOT_DRIVER(pcie_xilinx) = {
On Wed, May 10, 2017 at 11:10 AM, Fabio Estevam <festevam@gmail.com> wrote: > Hi Tim, > > Thanks for working on this! > > [Adding Peter and Jagan on Cc] > > On Wed, May 10, 2017 at 1:43 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 was tested on a Gateworks Ventana board. >> >> TODO: what is the right arch specific place to hook the remove function? > > Ideally the pcie_imx.c driver should be converted to driver model and > then the .remove can be simply added, just like in Barebox. > Fabio, Unfortunately converting the IMX PCI driver to DM is a cleanup effort that I'm not permitted the time to do. Regards, Tim
On Thu, May 11, 2017 at 6:01 AM, Peter Senna Tschudin <peter.senna@collabora.com> wrote: > Thank you for working on this! Your patch worked for me, but I needed to > add an "extern void imx_pcie_remove(void);" before calling the function, > which probably means I'm missing something. The extern should be added into a header file. > After applying this patch to u-boot, u-boot can initialize PCI and > 4.11.0-next-20170510 will boot. Without your patch 4.11.0-next-20170510 > do not boot if u-boot initialize the PCI bus. > > Thanks a lot! > > Here is the patch I'm using for our board: Stefano, The patches that Tim/Peter posted solve a kernel hang with kernel 4.11 on mx6q when U-Boot has PCI support. Could you please take a look and let us know if there is a more generic way to fix this issue, like fixing it for all mx6qdl boards that have PCI support? I understand that ideally the imx6 pci driver should be converted to driver model and then we add the .remove hook, like Lucas did for Barebox: https://git.pengutronix.de/cgit/barebox/commit/?id=f1da98da2760c21487bbba8f7fb957c843a22896 While the imx pci driver is not converted to device model, would you be willing to accept such per board patches for the time being? Please advise. Thanks > > diff --git a/board/ge/bx50v3/bx50v3.c b/board/ge/bx50v3/bx50v3.c > index cb49b58..7e03082 100644 > --- a/board/ge/bx50v3/bx50v3.c > +++ b/board/ge/bx50v3/bx50v3.c > @@ -829,6 +829,7 @@ static void remove_ethaddr_env_var(int index) > setenv(env_var_name, NULL); > } > > +extern void imx_pcie_remove(void); > int last_stage_init(void) { > int i; > > @@ -838,6 +839,8 @@ int last_stage_init(void) { > remove_ethaddr_env_var(i); > } > > + imx_pcie_remove(); > + > return 0; > } > > 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) > {
On 05/11/2017 01:06 PM, Fabio Estevam wrote: > On Thu, May 11, 2017 at 6:01 AM, Peter Senna Tschudin > <peter.senna@collabora.com> wrote: > >> Thank you for working on this! Your patch worked for me, but I needed to >> add an "extern void imx_pcie_remove(void);" before calling the function, >> which probably means I'm missing something. > > The extern should be added into a header file. > >> After applying this patch to u-boot, u-boot can initialize PCI and >> 4.11.0-next-20170510 will boot. Without your patch 4.11.0-next-20170510 >> do not boot if u-boot initialize the PCI bus. >> >> Thanks a lot! >> >> Here is the patch I'm using for our board: > > Stefano, > > The patches that Tim/Peter posted solve a kernel hang with kernel 4.11 > on mx6q when U-Boot has PCI support. > > Could you please take a look and let us know if there is a more > generic way to fix this issue, like fixing it for all mx6qdl boards > that have PCI support? > > I understand that ideally the imx6 pci driver should be converted to > driver model and then we add the .remove hook, like Lucas did for > Barebox: > https://git.pengutronix.de/cgit/barebox/commit/?id=f1da98da2760c21487bbba8f7fb957c843a22896 > > While the imx pci driver is not converted to device model, would you > be willing to accept such per board patches for the time being? The DM conversion should be pretty trivial IMO, no ?
On Thu, May 11, 2017 at 8:47 AM, Marek Vasut <marex@denx.de> wrote:
> The DM conversion should be pretty trivial IMO, no ?
Any volunteers? ;-)
On 05/11/2017 03:08 PM, Fabio Estevam wrote: > On Thu, May 11, 2017 at 8:47 AM, Marek Vasut <marex@denx.de> wrote: > >> The DM conversion should be pretty trivial IMO, no ? > > Any volunteers? ;-) > Tim ? :)
Hi Fabio, On 11/05/2017 13:06, Fabio Estevam wrote: > On Thu, May 11, 2017 at 6:01 AM, Peter Senna Tschudin > <peter.senna@collabora.com> wrote: > >> Thank you for working on this! Your patch worked for me, but I needed to >> add an "extern void imx_pcie_remove(void);" before calling the function, >> which probably means I'm missing something. > > The extern should be added into a header file. > >> After applying this patch to u-boot, u-boot can initialize PCI and >> 4.11.0-next-20170510 will boot. Without your patch 4.11.0-next-20170510 >> do not boot if u-boot initialize the PCI bus. >> >> Thanks a lot! >> >> Here is the patch I'm using for our board: > > Stefano, > > The patches that Tim/Peter posted solve a kernel hang with kernel 4.11 > on mx6q when U-Boot has PCI support. > > Could you please take a look and let us know if there is a more > generic way to fix this issue, like fixing it for all mx6qdl boards > that have PCI support? > > I understand that ideally the imx6 pci driver should be converted to > driver model and then we add the .remove hook, like Lucas did for > Barebox: > https://git.pengutronix.de/cgit/barebox/commit/?id=f1da98da2760c21487bbba8f7fb957c843a22896 > > While the imx pci driver is not converted to device model, would you > be willing to accept such per board patches for the time being? > > Please advise. Ok - let wait for Tim's answer, if he has enough time to work on this to move to DM. If not, it will be ok for now to fix it in this way, hoping to move soon to DM. Regards, Stefano
On Thu, May 11, 2017 at 6:48 PM, Stefano Babic <sbabic@denx.de> wrote: > Hi Fabio, > > On 11/05/2017 13:06, Fabio Estevam wrote: >> On Thu, May 11, 2017 at 6:01 AM, Peter Senna Tschudin >> <peter.senna@collabora.com> wrote: >> >>> Thank you for working on this! Your patch worked for me, but I needed to >>> add an "extern void imx_pcie_remove(void);" before calling the function, >>> which probably means I'm missing something. >> >> The extern should be added into a header file. >> >>> After applying this patch to u-boot, u-boot can initialize PCI and >>> 4.11.0-next-20170510 will boot. Without your patch 4.11.0-next-20170510 >>> do not boot if u-boot initialize the PCI bus. >>> >>> Thanks a lot! >>> >>> Here is the patch I'm using for our board: >> >> Stefano, >> >> The patches that Tim/Peter posted solve a kernel hang with kernel 4.11 >> on mx6q when U-Boot has PCI support. >> >> Could you please take a look and let us know if there is a more >> generic way to fix this issue, like fixing it for all mx6qdl boards >> that have PCI support? >> >> I understand that ideally the imx6 pci driver should be converted to >> driver model and then we add the .remove hook, like Lucas did for >> Barebox: >> https://git.pengutronix.de/cgit/barebox/commit/?id=f1da98da2760c21487bbba8f7fb957c843a22896 >> >> While the imx pci driver is not converted to device model, would you >> be willing to accept such per board patches for the time being? >> >> Please advise. > > > Ok - let wait for Tim's answer, if he has enough time to work on this to > move to DM. If not, it will be ok for now to fix it in this way, hoping > to move soon to DM. If none, have time to work, I'll work for dm conversion and have board to test the same. Jagan.
On Thu, May 11, 2017 at 6:18 AM, Stefano Babic <sbabic@denx.de> wrote: > Hi Fabio, > > On 11/05/2017 13:06, Fabio Estevam wrote: >> On Thu, May 11, 2017 at 6:01 AM, Peter Senna Tschudin >> <peter.senna@collabora.com> wrote: >> >>> Thank you for working on this! Your patch worked for me, but I needed to >>> add an "extern void imx_pcie_remove(void);" before calling the function, >>> which probably means I'm missing something. >> >> The extern should be added into a header file. >> >>> After applying this patch to u-boot, u-boot can initialize PCI and >>> 4.11.0-next-20170510 will boot. Without your patch 4.11.0-next-20170510 >>> do not boot if u-boot initialize the PCI bus. >>> >>> Thanks a lot! >>> >>> Here is the patch I'm using for our board: >> >> Stefano, >> >> The patches that Tim/Peter posted solve a kernel hang with kernel 4.11 >> on mx6q when U-Boot has PCI support. >> >> Could you please take a look and let us know if there is a more >> generic way to fix this issue, like fixing it for all mx6qdl boards >> that have PCI support? >> >> I understand that ideally the imx6 pci driver should be converted to >> driver model and then we add the .remove hook, like Lucas did for >> Barebox: >> https://git.pengutronix.de/cgit/barebox/commit/?id=f1da98da2760c21487bbba8f7fb957c843a22896 >> >> While the imx pci driver is not converted to device model, would you >> be willing to accept such per board patches for the time being? >> >> Please advise. > > > Ok - let wait for Tim's answer, if he has enough time to work on this to > move to DM. If not, it will be ok for now to fix it in this way, hoping > to move soon to DM. > > Regards, > Stefano Stefano, I don't have enough time to work on the DM conversion currently but it sounds like this would be a welcomed 'fix' that we should get in sooner than later. I know the include is missing from a header but I didn't bother as I though there would be a better arch-specific place to put the call to the remove. I don't think putting it in ft_board_setup made sense. Do you have any suggestions where it should go? Tim
diff --git a/board/gateworks/gw_ventana/gw_ventana.c b/board/gateworks/gw_ventana/gw_ventana.c index f404497..6762a71 100644 --- a/board/gateworks/gw_ventana/gw_ventana.c +++ b/board/gateworks/gw_ventana/gw_ventana.c @@ -1163,6 +1163,9 @@ int ft_board_setup(void *blob, bd_t *bd) int i; char rev = 0; +#if defined(CONFIG_CMD_PCI) + imx_pcie_remove(); +#endif /* determine board revision */ for (i = sizeof(ventana_info.model) - 1; i > 0; i--) { if (ventana_info.model[i] >= 'A') { 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) {
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 was tested on a Gateworks Ventana board. TODO: what is the right arch specific place to hook the remove function? Signed-off-by: Tim Harvey <tharvey@gateworks.com> --- board/gateworks/gw_ventana/gw_ventana.c | 3 +++ drivers/pci/pcie_imx.c | 38 +++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+)