[U-Boot] [PATCH] pci: mx6: Implement reset callback

[U-Boot] [PATCH] pci: mx6: Implement reset callback

[Marek Vasut]

Add a callback so that a board can implement it's own specific routine to
toggle the port's nRESET line.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Stefano Babic <sbabic@denx.de>
---
 drivers/pci/pcie_imx.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pcie_imx.c b/drivers/pci/pcie_imx.c
index 0a74867..b554075 100644
--- a/drivers/pci/pcie_imx.c
+++ b/drivers/pci/pcie_imx.c
@@ -450,6 +450,13 @@ static int imx6_pcie_init_phy(void)
 	return 0;
 }
 
+__weak int imx6_pcie_toggle_reset(void)
+{
+	/* This function ought to be overridden ! */
+	puts("WARNING: Make sure the PCIe nRESET line is connected!\n");
+	return 0;
+}
+
 static int imx6_pcie_deassert_core_reset(void)
 {
 	struct iomuxc *iomuxc_regs = (struct iomuxc *)IOMUXC_BASE_ADDR;
@@ -466,10 +473,9 @@ static int imx6_pcie_deassert_core_reset(void)
 	 * Wait for the clock to settle a bit, when the clock are sourced
 	 * from the CPU, we need about 30mS to settle.
 	 */
-	mdelay(30);
+	mdelay(50);
 
-	/* FIXME: GPIO reset goes here */
-	mdelay(100);
+	imx6_pcie_toggle_reset();
 
 	return 0;
 }
-- 
1.8.4.2

[U-Boot] [PATCH] pci: mx6: Implement reset callback

[Stefano Babic]

Hi Marek,

On 24/01/2014 16:25, Marek Vasut wrote:
> Add a callback so that a board can implement it's own specific routine to
> toggle the port's nRESET line.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Stefano Babic <sbabic@denx.de>
> ---
>  drivers/pci/pcie_imx.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pcie_imx.c b/drivers/pci/pcie_imx.c
> index 0a74867..b554075 100644
> --- a/drivers/pci/pcie_imx.c
> +++ b/drivers/pci/pcie_imx.c
> @@ -450,6 +450,13 @@ static int imx6_pcie_init_phy(void)
>  	return 0;
>  }
>  
> +__weak int imx6_pcie_toggle_reset(void)
> +{
> +	/* This function ought to be overridden ! */
> +	puts("WARNING: Make sure the PCIe nRESET line is connected!\n");
> +	return 0;
> +}
> +

Just to know: I assume that the nRESET is implemented with a GPIO. I am
expecting then in the board files a diffusion of imx6_pcie_toggle_reset,
where the oinly difference is the number of GPIO.

What about to define the GPIO in the board configuration file instead of
defininig the function as __weak ?

Something like

imx6_pcie_toggle_reset(int nReset)
{
..
}

static int imx6_pcie_deassert_core_reset(void)
{

...
	imx6_pcie_toggle_reset(CONFIG_IMX6_PCIE_RESET);

}

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
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
=====================================================================

[U-Boot] [PATCH] pci: mx6: Implement reset callback

[Marek Vasut]

On Tuesday, January 28, 2014 at 04:06:12 PM, Stefano Babic wrote:
> Hi Marek,
> 
> On 24/01/2014 16:25, Marek Vasut wrote:
> > Add a callback so that a board can implement it's own specific routine to
> > toggle the port's nRESET line.
> > 
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Stefano Babic <sbabic@denx.de>
> > ---
> > 
> >  drivers/pci/pcie_imx.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pci/pcie_imx.c b/drivers/pci/pcie_imx.c
> > index 0a74867..b554075 100644
> > --- a/drivers/pci/pcie_imx.c
> > +++ b/drivers/pci/pcie_imx.c
> > @@ -450,6 +450,13 @@ static int imx6_pcie_init_phy(void)
> > 
> >  	return 0;
> >  
> >  }
> > 
> > +__weak int imx6_pcie_toggle_reset(void)
> > +{
> > +	/* This function ought to be overridden ! */
> > +	puts("WARNING: Make sure the PCIe nRESET line is connected!\n");
> > +	return 0;
> > +}
> > +
> 
> Just to know: I assume that the nRESET is implemented with a GPIO.

Yes, that's how it is on all designs I saw thus far (but see below).

> I am
> expecting then in the board files a diffusion of imx6_pcie_toggle_reset,
> where the oinly difference is the number of GPIO.

The problem is, there are boards with no nRESET connected to the slot. These 
boards are broken, thus will print the warning message.

> What about to define the GPIO in the board configuration file instead of
> defininig the function as __weak ?

I am hell-bent on printing the warning message there btw.. Such broken designs 
should be caught early on.

> Something like
> 
> imx6_pcie_toggle_reset(int nReset)
> {
> ..
> }
> 
> static int imx6_pcie_deassert_core_reset(void)
> {
> 
> ...
> 	imx6_pcie_toggle_reset(CONFIG_IMX6_PCIE_RESET);
> 
> }
> 
> Best regards,
> Stefano Babic

Best regards,
Marek Vasut

[U-Boot] [PATCH] pci: mx6: Implement reset callback

[Stefano Babic]

Hi Marek,

sorry for late answer.

On 28/01/2014 20:32, Marek Vasut wrote:
> On Tuesday, January 28, 2014 at 04:06:12 PM, Stefano Babic wrote:
>> Hi Marek,
>>
>> On 24/01/2014 16:25, Marek Vasut wrote:
>>> Add a callback so that a board can implement it's own specific routine to
>>> toggle the port's nRESET line.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Cc: Stefano Babic <sbabic@denx.de>
>>> ---
>>>
>>>  drivers/pci/pcie_imx.c | 12 +++++++++---
>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/pci/pcie_imx.c b/drivers/pci/pcie_imx.c
>>> index 0a74867..b554075 100644
>>> --- a/drivers/pci/pcie_imx.c
>>> +++ b/drivers/pci/pcie_imx.c
>>> @@ -450,6 +450,13 @@ static int imx6_pcie_init_phy(void)
>>>
>>>  	return 0;
>>>  
>>>  }
>>>
>>> +__weak int imx6_pcie_toggle_reset(void)
>>> +{
>>> +	/* This function ought to be overridden ! */
>>> +	puts("WARNING: Make sure the PCIe nRESET line is connected!\n");
>>> +	return 0;
>>> +}
>>> +
>>
>> Just to know: I assume that the nRESET is implemented with a GPIO.
> 
> Yes, that's how it is on all designs I saw thus far (but see below).
> 
>> I am
>> expecting then in the board files a diffusion of imx6_pcie_toggle_reset,
>> where the oinly difference is the number of GPIO.
> 
> The problem is, there are boards with no nRESET connected to the slot.

Any reference to the Sabrelite is, of course, purely coincidental. But
this is a hardware bug on a specific board and we should not adjust all
boards according to the broken one.

What do you think to check the validity of the GPIO ? For example,
setting the GPIO to -1 for sabrelite and printing the message if the
GPIO is negative or not defined ?

> These 
> boards are broken, thus will print the warning message.

Right - but there is nothing we can do it. The hardware must be fixed.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
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
=====================================================================

[U-Boot] [PATCH] pci: mx6: Implement reset callback

[Marek Vasut]

On Monday, February 03, 2014 at 12:56:04 PM, Stefano Babic wrote:
> Hi Marek,
> 
> sorry for late answer.
> 
> On 28/01/2014 20:32, Marek Vasut wrote:
> > On Tuesday, January 28, 2014 at 04:06:12 PM, Stefano Babic wrote:
> >> Hi Marek,
> >> 
> >> On 24/01/2014 16:25, Marek Vasut wrote:
> >>> Add a callback so that a board can implement it's own specific routine
> >>> to toggle the port's nRESET line.
> >>> 
> >>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>> Cc: Stefano Babic <sbabic@denx.de>
> >>> ---
> >>> 
> >>>  drivers/pci/pcie_imx.c | 12 +++++++++---
> >>>  1 file changed, 9 insertions(+), 3 deletions(-)
> >>> 
> >>> diff --git a/drivers/pci/pcie_imx.c b/drivers/pci/pcie_imx.c
> >>> index 0a74867..b554075 100644
> >>> --- a/drivers/pci/pcie_imx.c
> >>> +++ b/drivers/pci/pcie_imx.c
> >>> @@ -450,6 +450,13 @@ static int imx6_pcie_init_phy(void)
> >>> 
> >>>  	return 0;
> >>>  
> >>>  }
> >>> 
> >>> +__weak int imx6_pcie_toggle_reset(void)
> >>> +{
> >>> +	/* This function ought to be overridden ! */
> >>> +	puts("WARNING: Make sure the PCIe nRESET line is connected!\n");
> >>> +	return 0;
> >>> +}
> >>> +
> >> 
> >> Just to know: I assume that the nRESET is implemented with a GPIO.
> > 
> > Yes, that's how it is on all designs I saw thus far (but see below).
> > 
> >> I am
> >> expecting then in the board files a diffusion of imx6_pcie_toggle_reset,
> >> where the oinly difference is the number of GPIO.
> > 
> > The problem is, there are boards with no nRESET connected to the slot.
> 
> Any reference to the Sabrelite is, of course, purely coincidental. But
> this is a hardware bug on a specific board and we should not adjust all
> boards according to the broken one.

Well ... SL and N6X both. For all I care, we can have #define 
MX6_PCIE_RESET_GPIO and if that's not defined, puke out this warning. And 
ultimatelly let this function be overriden anyway in case people used some GPIO 
expander or whatnot. So the change to this would be:

__weak int imx6_pcie_toggle_reset(void)
{
#ifdef CONFIG_MX6_PCIE_RESET_GPIO
 gpio_set...
 mdelay();
 gpio_set...
 mdelay();
#else
 puts("Oh yeah, broken design :-(\n");
#endif
}

This should effectivelly give you all the flexibility, what do you say?

> What do you think to check the validity of the GPIO ? For example,
> setting the GPIO to -1 for sabrelite and printing the message if the
> GPIO is negative or not defined ?
> 
> > These
> > boards are broken, thus will print the warning message.
> 
> Right - but there is nothing we can do it. The hardware must be fixed.

CCing Troy, Eric, Tim, surely this will make their day ;-)

Best regards,
Marek Vasut

[U-Boot] [PATCH] pci: mx6: Implement reset callback

[Eric Nelson]

Hi Marek,

On 02/03/2014 11:17 AM, Marek Vasut wrote:
> On Monday, February 03, 2014 at 12:56:04 PM, Stefano Babic wrote:
>> Hi Marek,
>>
>> sorry for late answer.
>>
>> On 28/01/2014 20:32, Marek Vasut wrote:
>>> On Tuesday, January 28, 2014 at 04:06:12 PM, Stefano Babic wrote:
>>>> Hi Marek,
>>>>
>>>> On 24/01/2014 16:25, Marek Vasut wrote:
>>>>> Add a callback so that a board can implement it's own specific routine
>>>>> to toggle the port's nRESET line.
>>>>>
>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>> Cc: Stefano Babic <sbabic@denx.de>
>>>>> ---
>>>>>
>>>>>   drivers/pci/pcie_imx.c | 12 +++++++++---
>>>>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/pcie_imx.c b/drivers/pci/pcie_imx.c
>>>>> index 0a74867..b554075 100644
>>>>> --- a/drivers/pci/pcie_imx.c
>>>>> +++ b/drivers/pci/pcie_imx.c
>>>>> @@ -450,6 +450,13 @@ static int imx6_pcie_init_phy(void)
>>>>>
>>>>>   	return 0;
>>>>>
>>>>>   }
>>>>>
>>>>> +__weak int imx6_pcie_toggle_reset(void)
>>>>> +{
>>>>> +	/* This function ought to be overridden ! */
>>>>> +	puts("WARNING: Make sure the PCIe nRESET line is connected!\n");
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>
>>>> Just to know: I assume that the nRESET is implemented with a GPIO.
>>>
>>> Yes, that's how it is on all designs I saw thus far (but see below).
>>>
>>>> I am
>>>> expecting then in the board files a diffusion of imx6_pcie_toggle_reset,
>>>> where the oinly difference is the number of GPIO.
>>>
>>> The problem is, there are boards with no nRESET connected to the slot.
>>
>> Any reference to the Sabrelite is, of course, purely coincidental. But
>> this is a hardware bug on a specific board and we should not adjust all
>> boards according to the broken one.
>
> Well ... SL and N6X both. For all I care, we can have #define
> MX6_PCIE_RESET_GPIO and if that's not defined, puke out this warning. And
> ultimatelly let this function be overriden anyway in case people used some GPIO
> expander or whatnot. So the change to this would be:
>
> __weak int imx6_pcie_toggle_reset(void)
> {
> #ifdef CONFIG_MX6_PCIE_RESET_GPIO
>   gpio_set...
>   mdelay();
>   gpio_set...
>   mdelay();
> #else
>   puts("Oh yeah, broken design :-(\n");

That's pretty harsh!

We have lots of stuff working without a GPIO...

[U-Boot] [PATCH] pci: mx6: Implement reset callback

[Marek Vasut]

On Monday, February 03, 2014 at 07:40:09 PM, Eric Nelson wrote:

[...]

> > Well ... SL and N6X both. For all I care, we can have #define
> > MX6_PCIE_RESET_GPIO and if that's not defined, puke out this warning. And
> > ultimatelly let this function be overriden anyway in case people used
> > some GPIO expander or whatnot. So the change to this would be:
> > 
> > __weak int imx6_pcie_toggle_reset(void)
> > {
> > #ifdef CONFIG_MX6_PCIE_RESET_GPIO
> > 
> >   gpio_set...
> >   mdelay();
> >   gpio_set...
> >   mdelay();
> > 
> > #else
> > 
> >   puts("Oh yeah, broken design :-(\n");
> 
> That's pretty harsh!

Yes, I know that won't please you :-(

> We have lots of stuff working without a GPIO...

Actually, see PCI Express Base Specification, Rev. 3.0 : Section 6.6.1 : 
Paragraph 2 . Quote:

"
6.6.1. Conventional Reset

Conventional Reset includes all reset mechanisms other than Function Level 
Reset. There are two categories of Conventional Resets: Fundamental Reset and 
resets that are not Fundamental Reset. This section applies to all types of 
Conventional Reset.

In all form factors and system hardware configurations, there must, at some 
level, be a hardware mechanism for setting or returning all Port states to the 
initial conditions specified in this document ? this mechanism is called 
?Fundamental Reset.? This mechanism can take the form of an auxiliary
signal provided by the system to a component or adapter card, in which case the 
signal must be called PERST#, and must conform to the rules specified in Section 
4.2.4.8.1. When PERST# is provided to a component or adapter, this signal must 
be used by the component or adapter as Fundamental Reset.

When PERST# is not provided to a component or adapter, Fundamental Reset is 
generated autonomously by the component or adapter, and the details of how this 
is done are outside the scope of this document. If a Fundamental Reset is 
generated autonomously by the component or adapter, and if power is supplied by 
the platform to the component/adapter, the component/adapter must generate a 
Fundamental Reset to itself if the supplied power goes outside of the limits 
specified for the form factor or system.
"

This means, your platform _MUST_ have a FR implementation. If you have PERST 
connected (that's the reset pin) to for example GPIO, then so be it and that's 
your FR.

The third paragraph states that if you do NOT have PERST connected, you need 
some other way of doing FR. Another way of generating FR is to depend on POR, so 
when power is applied to the component, it will generate FR internally. Thus to 
produce an "alternative" FR without PERST connected, you toggle the power GPIO 
of the particular slot.

I think the SL can do neither, right ? :-(

Best regards,
Marek Vasut

[U-Boot] [PATCH] pci: mx6: Implement reset callback

[Eric Nelson]

Hi Marek,

On 02/03/2014 12:33 PM, Marek Vasut wrote:
> On Monday, February 03, 2014 at 07:40:09 PM, Eric Nelson wrote:
>
> [...]
>
>>> Well ... SL and N6X both. For all I care, we can have #define
>>> MX6_PCIE_RESET_GPIO and if that's not defined, puke out this warning. And
>>> ultimatelly let this function be overriden anyway in case people used
>>> some GPIO expander or whatnot. So the change to this would be:
>>>
>>> __weak int imx6_pcie_toggle_reset(void)
>>> {
>>> #ifdef CONFIG_MX6_PCIE_RESET_GPIO
>>>
>>>    gpio_set...
>>>    mdelay();
>>>    gpio_set...
>>>    mdelay();
>>>
>>> #else
>>>
>>>    puts("Oh yeah, broken design :-(\n");
>>
>> That's pretty harsh!
>
> Yes, I know that won't please you :-(
>

Ouch!

>> We have lots of stuff working without a GPIO...
>
> Actually, see PCI Express Base Specification, Rev. 3.0 : Section 6.6.1 :
> Paragraph 2 . Quote:
>
> "
> 6.6.1. Conventional Reset
>
> Conventional Reset includes all reset mechanisms other than Function Level
> Reset. There are two categories of Conventional Resets: Fundamental Reset and
> resets that are not Fundamental Reset. This section applies to all types of
> Conventional Reset.
>
> In all form factors and system hardware configurations, there must, at some
> level, be a hardware mechanism for setting or returning all Port states to the
> initial conditions specified in this document ? this mechanism is called
> ?Fundamental Reset.? This mechanism can take the form of an auxiliary
> signal provided by the system to a component or adapter card, in which case the
> signal must be called PERST#, and must conform to the rules specified in Section
> 4.2.4.8.1. When PERST# is provided to a component or adapter, this signal must
> be used by the component or adapter as Fundamental Reset.
>
> When PERST# is not provided to a component or adapter, Fundamental Reset is
> generated autonomously by the component or adapter, and the details of how this
> is done are outside the scope of this document. If a Fundamental Reset is
> generated autonomously by the component or adapter, and if power is supplied by
> the platform to the component/adapter, the component/adapter must generate a
> Fundamental Reset to itself if the supplied power goes outside of the limits
> specified for the form factor or system.
> "
>
> This means, your platform _MUST_ have a FR implementation. If you have PERST
> connected (that's the reset pin) to for example GPIO, then so be it and that's
> your FR.
>
> The third paragraph states that if you do NOT have PERST connected, you need
> some other way of doing FR. Another way of generating FR is to depend on POR, so
> when power is applied to the component, it will generate FR internally. Thus to
> produce an "alternative" FR without PERST connected, you toggle the power GPIO
> of the particular slot.
>
> I think the SL can do neither, right ? :-(
>

Right again.

PCIe was very much an afterthought (and late addition) on SABRE Lite,
and unfortunately only slightly improved on Nitrogen6x.

We have had success in using/testing PCIe devices without either, but
that doesn't mean we match the spec, and I suppose we'll have to live
with the "broken design" message...

Regards,


Eric

[U-Boot] [PATCH] pci: mx6: Implement reset callback

[Marek Vasut]

On Monday, February 03, 2014 at 08:57:30 PM, Eric Nelson wrote:

[...]

> > "
> > 6.6.1. Conventional Reset
> > 
> > Conventional Reset includes all reset mechanisms other than Function
> > Level Reset. There are two categories of Conventional Resets:
> > Fundamental Reset and resets that are not Fundamental Reset. This
> > section applies to all types of Conventional Reset.
> > 
> > In all form factors and system hardware configurations, there must, at
> > some level, be a hardware mechanism for setting or returning all Port
> > states to the initial conditions specified in this document ? this
> > mechanism is called ?Fundamental Reset.? This mechanism can take the
> > form of an auxiliary signal provided by the system to a component or
> > adapter card, in which case the signal must be called PERST#, and must
> > conform to the rules specified in Section 4.2.4.8.1. When PERST# is
> > provided to a component or adapter, this signal must be used by the
> > component or adapter as Fundamental Reset.
> > 
> > When PERST# is not provided to a component or adapter, Fundamental Reset
> > is generated autonomously by the component or adapter, and the details
> > of how this is done are outside the scope of this document. If a
> > Fundamental Reset is generated autonomously by the component or adapter,
> > and if power is supplied by the platform to the component/adapter, the
> > component/adapter must generate a Fundamental Reset to itself if the
> > supplied power goes outside of the limits specified for the form factor
> > or system.
> > "
> > 
> > This means, your platform _MUST_ have a FR implementation. If you have
> > PERST connected (that's the reset pin) to for example GPIO, then so be
> > it and that's your FR.
> > 
> > The third paragraph states that if you do NOT have PERST connected, you
> > need some other way of doing FR. Another way of generating FR is to
> > depend on POR, so when power is applied to the component, it will
> > generate FR internally. Thus to produce an "alternative" FR without
> > PERST connected, you toggle the power GPIO of the particular slot.
> > 
> > I think the SL can do neither, right ? :-(
> 
> Right again.
> 
> PCIe was very much an afterthought (and late addition) on SABRE Lite,
> and unfortunately only slightly improved on Nitrogen6x.

OK. This wasn't an attack in the SL direction, really. I really want to warn 
people to comply with the spec, since otherwise they will suffer from weird bugs 
that are hard to find. The PERST is a prime example of that :-(

We really should start waving a sign "YOU MUST CONNECT PERST IN YOUR NEW DESIGN,
OTHERWISE A KITTEN DIES!"

> We have had success in using/testing PCIe devices without either, but
> that doesn't mean we match the spec, and I suppose we'll have to live
> with the "broken design" message...

I know. The design without FR works most of the time, but there is one 
particular scenario where it may fail (means it fails reliably). I will assume 
we have just a simple RC<->EP connection with EP being i82574L card (well 
supported and easily available intel NIC):

1) Cold boot the system
2) Bring up the PCIe link in U-Boot
3) Use the e1000e driver for some transfer
4) Boot Linux
5) Bring up the PCIe link in Linux
6) Use the e1000e driver for some transfer
7) Reboot the system from Linux
8) Bring up the PCIe link in U-Boot

In case you don't have means to do FR, your system will fail during 5) and/or 
during 8) because in either case, the link and/or EP device can be in undefined 
state from previous usage. You are therefore not able to send in-band messages 
to the EP (to issue hot reset for example*) nor restart the link, thus you're 
trapped.

* if you try to send anything over unstable PCIe link on MX6, it can stall your 
entire system to the point where the system bus is stuck and not even JTAG 
debugger can halt the CPU (!)

Best regards,
Marek Vasut

[U-Boot] [PATCH] pci: mx6: Implement reset callback

[Eric Nelson]

Hi Marek,

On 02/03/2014 01:16 PM, Marek Vasut wrote:
> On Monday, February 03, 2014 at 08:57:30 PM, Eric Nelson wrote:
>
> [...]
>

I like having this bit included, but do you need to attribute copyright
for this block?

>>> "
>>> 6.6.1. Conventional Reset
>>>
>>> Conventional Reset includes all reset mechanisms other than Function
>>> Level Reset. There are two categories of Conventional Resets:
>>> Fundamental Reset and resets that are not Fundamental Reset. This
>>> section applies to all types of Conventional Reset.
>>>
>>> In all form factors and system hardware configurations, there must, at
>>> some level, be a hardware mechanism for setting or returning all Port
>>> states to the initial conditions specified in this document ? this
>>> mechanism is called ?Fundamental Reset.? This mechanism can take the
>>> form of an auxiliary signal provided by the system to a component or
>>> adapter card, in which case the signal must be called PERST#, and must
>>> conform to the rules specified in Section 4.2.4.8.1. When PERST# is
>>> provided to a component or adapter, this signal must be used by the
>>> component or adapter as Fundamental Reset.
>>>
>>> When PERST# is not provided to a component or adapter, Fundamental Reset
>>> is generated autonomously by the component or adapter, and the details
>>> of how this is done are outside the scope of this document. If a
>>> Fundamental Reset is generated autonomously by the component or adapter,
>>> and if power is supplied by the platform to the component/adapter, the
>>> component/adapter must generate a Fundamental Reset to itself if the
>>> supplied power goes outside of the limits specified for the form factor
>>> or system.
>>> "
>>>
>>> This means, your platform _MUST_ have a FR implementation. If you have
>>> PERST connected (that's the reset pin) to for example GPIO, then so be
>>> it and that's your FR.
>>>
>>> The third paragraph states that if you do NOT have PERST connected, you
>>> need some other way of doing FR. Another way of generating FR is to
>>> depend on POR, so when power is applied to the component, it will
>>> generate FR internally. Thus to produce an "alternative" FR without
>>> PERST connected, you toggle the power GPIO of the particular slot.
>>>
>>> I think the SL can do neither, right ? :-(
>>
>> Right again.
>>
>> PCIe was very much an afterthought (and late addition) on SABRE Lite,
>> and unfortunately only slightly improved on Nitrogen6x.
>
> OK. This wasn't an attack in the SL direction, really. I really want to warn
> people to comply with the spec, since otherwise they will suffer from weird bugs
> that are hard to find. The PERST is a prime example of that :-(
>

No worries. I appreciate the details.

> We really should start waving a sign "YOU MUST CONNECT PERST IN YOUR NEW DESIGN,
> OTHERWISE A KITTEN DIES!"
>

Hey, leave the kittens alone!

>> We have had success in using/testing PCIe devices without either, but
>> that doesn't mean we match the spec, and I suppose we'll have to live
>> with the "broken design" message...
>
> I know. The design without FR works most of the time, but there is one
> particular scenario where it may fail (means it fails reliably). I will assume
> we have just a simple RC<->EP connection with EP being i82574L card (well
> supported and easily available intel NIC):
>
> 1) Cold boot the system
> 2) Bring up the PCIe link in U-Boot
> 3) Use the e1000e driver for some transfer
> 4) Boot Linux
> 5) Bring up the PCIe link in Linux
> 6) Use the e1000e driver for some transfer
> 7) Reboot the system from Linux
> 8) Bring up the PCIe link in U-Boot
>
> In case you don't have means to do FR, your system will fail during 5) and/or
> during 8) because in either case, the link and/or EP device can be in undefined
> state from previous usage. You are therefore not able to send in-band messages
> to the EP (to issue hot reset for example*) nor restart the link, thus you're
> trapped.
>
> * if you try to send anything over unstable PCIe link on MX6, it can stall your
> entire system to the point where the system bus is stuck and not even JTAG
> debugger can halt the CPU (!)
>

Thanks. That's a useful test scenario.

[U-Boot] [PATCH] pci: mx6: Implement reset callback

[Marek Vasut]

On Monday, February 03, 2014 at 09:54:33 PM, Eric Nelson wrote:
> Hi Marek,
> 
> On 02/03/2014 01:16 PM, Marek Vasut wrote:
> > On Monday, February 03, 2014 at 08:57:30 PM, Eric Nelson wrote:
> > 
> > [...]
> 
> I like having this bit included, but do you need to attribute copyright
> for this block?

I'd hate to tempt PCISIG with this portion actually. That's why I didn't put the 
whole quotation into the patch either.

[...]

> > We really should start waving a sign "YOU MUST CONNECT PERST IN YOUR NEW
> > DESIGN, OTHERWISE A KITTEN DIES!"
> 
> Hey, leave the kittens alone!

Are ponies ok? Friendship is magic afterall :-)

> >> We have had success in using/testing PCIe devices without either, but
> >> that doesn't mean we match the spec, and I suppose we'll have to live
> >> with the "broken design" message...
> > 
> > I know. The design without FR works most of the time, but there is one
> > particular scenario where it may fail (means it fails reliably). I will
> > assume we have just a simple RC<->EP connection with EP being i82574L
> > card (well supported and easily available intel NIC):
> > 
> > 1) Cold boot the system
> > 2) Bring up the PCIe link in U-Boot
> > 3) Use the e1000e driver for some transfer
> > 4) Boot Linux
> > 5) Bring up the PCIe link in Linux
> > 6) Use the e1000e driver for some transfer
> > 7) Reboot the system from Linux
> > 8) Bring up the PCIe link in U-Boot
> > 
> > In case you don't have means to do FR, your system will fail during 5)
> > and/or during 8) because in either case, the link and/or EP device can
> > be in undefined state from previous usage. You are therefore not able to
> > send in-band messages to the EP (to issue hot reset for example*) nor
> > restart the link, thus you're trapped.
> > 
> > * if you try to send anything over unstable PCIe link on MX6, it can
> > stall your entire system to the point where the system bus is stuck and
> > not even JTAG debugger can halt the CPU (!)
> 
> Thanks. That's a useful test scenario.

Hope it helps :) Sometimes you need to do a few cycles until the hardware hangs. 
I found this out when I was debugging another custom board here. I think we will 
have a rock-solid PCIe implementation on MX6 for 3.14 and 2014.04 ;-)

Best regards,
Marek Vasut

[U-Boot] [PATCH] pci: mx6: Implement reset callback

[Eric Nelson]

On 02/03/2014 04:34 PM, Marek Vasut wrote:
> On Monday, February 03, 2014 at 09:54:33 PM, Eric Nelson wrote:
>> Hi Marek,
>>
>> On 02/03/2014 01:16 PM, Marek Vasut wrote:
>>> On Monday, February 03, 2014 at 08:57:30 PM, Eric Nelson wrote:
>>>
>>> [...]
>>
>> I like having this bit included, but do you need to attribute copyright
>> for this block?
>
> I'd hate to tempt PCISIG with this portion actually. That's why I didn't put the
> whole quotation into the patch either.
>
> [...]
>
>>> We really should start waving a sign "YOU MUST CONNECT PERST IN YOUR NEW
>>> DESIGN, OTHERWISE A KITTEN DIES!"
>>
>> Hey, leave the kittens alone!
>
> Are ponies ok? Friendship is magic afterall :-)
>
Works for me.

>>>> We have had success in using/testing PCIe devices without either, but
>>>> that doesn't mean we match the spec, and I suppose we'll have to live
>>>> with the "broken design" message...
>>>
>>> I know. The design without FR works most of the time, but there is one
>>> particular scenario where it may fail (means it fails reliably). I will
>>> assume we have just a simple RC<->EP connection with EP being i82574L
>>> card (well supported and easily available intel NIC):
>>>
>>> 1) Cold boot the system
>>> 2) Bring up the PCIe link in U-Boot
>>> 3) Use the e1000e driver for some transfer
>>> 4) Boot Linux
>>> 5) Bring up the PCIe link in Linux
>>> 6) Use the e1000e driver for some transfer
>>> 7) Reboot the system from Linux
>>> 8) Bring up the PCIe link in U-Boot
>>>
>>> In case you don't have means to do FR, your system will fail during 5)
>>> and/or during 8) because in either case, the link and/or EP device can
>>> be in undefined state from previous usage. You are therefore not able to
>>> send in-band messages to the EP (to issue hot reset for example*) nor
>>> restart the link, thus you're trapped.
>>>
>>> * if you try to send anything over unstable PCIe link on MX6, it can
>>> stall your entire system to the point where the system bus is stuck and
>>> not even JTAG debugger can halt the CPU (!)
>>
>> Thanks. That's a useful test scenario.
>
> Hope it helps :) Sometimes you need to do a few cycles until the hardware hangs.
> I found this out when I was debugging another custom board here. I think we will
> have a rock-solid PCIe implementation on MX6 for 3.14 and 2014.04 ;-)
>

You can bet on a knee-jerk reaction at design reviews here in Arizona...

Regards,


Eric

[U-Boot] [PATCH] pci: mx6: Implement reset callback

[Marek Vasut]

On Tuesday, February 04, 2014 at 01:57:48 AM, Eric Nelson wrote:
[...]
> > Hope it helps :) Sometimes you need to do a few cycles until the hardware
> > hangs. I found this out when I was debugging another custom board here.
> > I think we will have a rock-solid PCIe implementation on MX6 for 3.14
> > and 2014.04 ;-)
> 
> You can bet on a knee-jerk reaction at design reviews here in Arizona...

Hehe :-)

Best regards,
Marek Vasut

[U-Boot] [PATCH] pci: mx6: Implement reset callback

[Stefano Babic]

Hi Marek,

On 03/02/2014 19:17, Marek Vasut wrote:
>
> Well ... SL and N6X both. For all I care, we can have #define
> MX6_PCIE_RESET_GPIO and if that's not defined, puke out this warning. And
> ultimatelly let this function be overriden anyway in case people used some GPIO
> expander or whatnot. So the change to this would be:
>
> __weak int imx6_pcie_toggle_reset(void)
> {
> #ifdef CONFIG_MX6_PCIE_RESET_GPIO
>   gpio_set...
>   mdelay();
>   gpio_set...
>   mdelay();
> #else
>   puts("Oh yeah, broken design :-(\n");
> #endif
> }
>
> This should effectivelly give you all the flexibility, what do you say?

Right. Go on !

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
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
=====================================================================

[U-Boot] [PATCH] pci: mx6: Implement reset callback

[Tim Harvey]

On Fri, Jan 24, 2014 at 7:25 AM, Marek Vasut <marex@denx.de> wrote:
> Add a callback so that a board can implement it's own specific routine to
> toggle the port's nRESET line.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Stefano Babic <sbabic@denx.de>
> ---
>  drivers/pci/pcie_imx.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/pcie_imx.c b/drivers/pci/pcie_imx.c
> index 0a74867..b554075 100644
> --- a/drivers/pci/pcie_imx.c
> +++ b/drivers/pci/pcie_imx.c
> @@ -450,6 +450,13 @@ static int imx6_pcie_init_phy(void)
>         return 0;
>  }
>
> +__weak int imx6_pcie_toggle_reset(void)
> +{
> +       /* This function ought to be overridden ! */
> +       puts("WARNING: Make sure the PCIe nRESET line is connected!\n");
> +       return 0;
> +}
> +
>  static int imx6_pcie_deassert_core_reset(void)
>  {
>         struct iomuxc *iomuxc_regs = (struct iomuxc *)IOMUXC_BASE_ADDR;
> @@ -466,10 +473,9 @@ static int imx6_pcie_deassert_core_reset(void)
>          * Wait for the clock to settle a bit, when the clock are sourced
>          * from the CPU, we need about 30mS to settle.
>          */
> -       mdelay(30);
> +       mdelay(50);
>
> -       /* FIXME: GPIO reset goes here */
> -       mdelay(100);
> +       imx6_pcie_toggle_reset();
>
>         return 0;
>  }

Tested-by: Tim Harvey <tharvey@gateworks.com>
Acked-by: Tim Harvey <tharvey@gateworks.com>

Tested on a Gateworks Ventana GW54xx board with a PLX PEX8609 switch
on the RC with 2 devices behind it:
  00:01.0     - 16c3:abcd - Bridge device
   01:00.0    - 10b5:8609 - Bridge device
    02:01.0   - 10b5:8609 - Bridge device
    02:04.0   - 10b5:8609 - Bridge device
    02:05.0   - 10b5:8609 - Bridge device
    02:06.0   - 10b5:8609 - Bridge device
    02:07.0   - 10b5:8609 - Bridge device
    02:08.0   - 10b5:8609 - Bridge device
     08:00.0  - 11ab:4380 - Network controller
    02:09.0   - 10b5:8609 - Bridge device
   01:00.1    - 10b5:8609 - Base system peripheral


I would love to see this merged as the PCI driver won't really work
without it for boards that have a proper PCI_RST#.

Thanks Merek!

Tim