Re: [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function

[Fabio Estevam]

Hi David,

On Fri, Sep 22, 2017 at 11:00 AM, 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?

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

Please give it a try.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function

[Tim Harvey]

On Fri, Sep 22, 2017 at 7:00 AM, David Müller (ELSOFT AG)
 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(_regs->gpr[1]);
>> + gpr12 = readl(_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, _regs->gpr[12]);
>
> I think this should be
> writel(gpr12, _regs->gpr[12]);
>
> or even better
> clrbits_le32(_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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function

[Marek Vasut]

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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function

[ELSOFT AG]

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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function

[Marek Vasut]

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(_regs->gpr[1]);
>> +gpr12 = readl(_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, _regs->gpr[12]);
> 
> I think this should be
>   writel(gpr12, _regs->gpr[12]);
> 
> or even better
>   clrbits_le32(_regs->gpr[12],
>   IOMUXC_GPR12_APPS_LTSSM_ENABLE);
> 
> as in the rest of the file.
> 
> Dave
> 


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function

[ELSOFT AG]

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(_regs->gpr[1]);
> + gpr12 = readl(_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, _regs->gpr[12]);

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

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

as in the rest of the file.

Dave
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function

[Stefano Babic]

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: sba...@denx.de
=
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function

[Stefano Babic]

Hi Tim,

On 18/05/2017 16:22, Tim Harvey wrote:
> On Thu, May 18, 2017 at 2:12 AM, Stefano Babic  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 
>>> ---
>>>  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(_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(_regs->gpr[1]);
>>> + gpr12 = readl(_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, _regs->gpr[12]);
>>> + }
>>> + }
>>>   setbits_le32(_regs->gpr[1], IOMUXC_GPR1_TEST_POWERDOWN);
>>>   clrbits_le32(_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 

Re: [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function

[Tim Harvey]

On Thu, May 18, 2017 at 2:12 AM, Stefano Babic  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 
>> ---
>>  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(_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(_regs->gpr[1]);
>> + gpr12 = readl(_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, _regs->gpr[12]);
>> + }
>> + }
>>   setbits_le32(_regs->gpr[1], IOMUXC_GPR1_TEST_POWERDOWN);
>>   clrbits_le32(_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
___
U-Boot mailing list
U-Boot@lists.denx.de

Re: [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function

[Stefano Babic]

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 
> ---
>  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(_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(_regs->gpr[1]);
> + gpr12 = readl(_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, _regs->gpr[12]);
> + }
> + }
>   setbits_le32(_regs->gpr[1], IOMUXC_GPR1_TEST_POWERDOWN);
>   clrbits_le32(_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: sba...@denx.de
=
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function

[Peter Senna Tschudin]

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  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 
> 
> This looks good:
> 
> Reviewed-by: Fabio Estevam 
> 
> 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 

> 
> > ---
> >  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(_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(_regs->gpr[1]);
> > +   gpr12 = readl(_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, _regs->gpr[12]);
> > +   }
> > +   }
> > setbits_le32(_regs->gpr[1], IOMUXC_GPR1_TEST_POWERDOWN);
> > clrbits_le32(_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
> >
___
U-Boot mailing list

Re: [U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function

[Fabio Estevam]

Hi Tim,

On Fri, May 12, 2017 at 4:58 PM, 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 

This looks good:

Reviewed-by: Fabio Estevam 

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(_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(_regs->gpr[1]);
> +   gpr12 = readl(_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, _regs->gpr[12]);
> +   }
> +   }
> setbits_le32(_regs->gpr[1], IOMUXC_GPR1_TEST_POWERDOWN);
> clrbits_le32(_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
>
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH] drivers: pci: imx: add imx_pcie_remove function

[Tim Harvey]

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 
---
 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(_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(_regs->gpr[1]);
+   gpr12 = readl(_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, _regs->gpr[12]);
+   }
+   }
setbits_le32(_regs->gpr[1], IOMUXC_GPR1_TEST_POWERDOWN);
clrbits_le32(_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

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot