[PATCH 0/1] ppc4xx: Fix PCIe scanning for the 460SX

[PATCH 0/1] ppc4xx: Fix PCIe scanning for the 460SX

[Ayman El-Khashab]

From: Ayman El-Khashab <ayman@elkhashab.com>

This patch is to fix the PCIe on the 460SX CPU.  As far as I can tell,
the 460SX must be using a different core than the previous 4xx SOCs.
The registers aren't the same and it appears DCRs that existed on 
previous parts don't exist on this one.  Perhaps somebody from AMCC 
can chime in.  In any case, the main problem was that the TX and RX
weren't enabled so nothing downstream from the RP was ever found.
And the check to set "port->link" would never work (as far as I could
tell).  I could not find any DCRs in this part to indicate when the
link was up and there isn't an obvious loopback, so the following
call would fail and result in no devices found.

ppc4xx_pciex_wait_on_sdr(port, PESDRn_LOOP, 1 << 28, 1 << 28, 100)

It would be better to detect the link status, but I don't see an
obvious way to do that from the data sheet.  U-boot just skips the
link check as well for this part.


Ayman El-Khashab (1):
  ppc4xx: Fix PCIe scanning for the 460SX

 arch/powerpc/sysdev/ppc4xx_pci.c |   35 +++++++++++++++++++++++++++++++----
 1 files changed, 31 insertions(+), 4 deletions(-)

-- 
1.7.4.3

[PATCH 1/1] ppc4xx: Fix PCIe scanning for the 460SX

[Ayman El-Khashab]

From: Ayman El-Khashab <ayman@elkhashab.com>

The 460SX uses a different register set than previous 44x PCIe CPUs,
so some of the checks were not valid.  Added an enable for the TX
and RX.  For the 460SX only: Bypassed VCO check and added PLL check.
Bypassed the link check.  Changed to advertise gen 2 speeds.

Signed-off-by: Ayman El-Khashab <ayman@elkhashab.com>
---
 arch/powerpc/sysdev/ppc4xx_pci.c |   35 +++++++++++++++++++++++++++++++----
 1 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/sysdev/ppc4xx_pci.c b/arch/powerpc/sysdev/ppc4xx_pci.c
index 156aa7d..1c854a7 100644
--- a/arch/powerpc/sysdev/ppc4xx_pci.c
+++ b/arch/powerpc/sysdev/ppc4xx_pci.c
@@ -1058,6 +1058,8 @@ static int __init ppc460sx_pciex_core_init(struct device_node *np)
 
 static int ppc460sx_pciex_init_port_hw(struct ppc4xx_pciex_port *port)
 {
+	/* 500ms should be long enough */
+	int timeout = 50000;
 
 	if (port->endpoint)
 		dcri_clrset(SDR0, port->sdr_base + PESDRn_UTLSET2,
@@ -1066,15 +1068,20 @@ static int ppc460sx_pciex_init_port_hw(struct ppc4xx_pciex_port *port)
 		dcri_clrset(SDR0, port->sdr_base + PESDRn_UTLSET2,
 				0, 0x01000000);
 
-	/*Gen-1*/
-	mtdcri(SDR0, port->sdr_base + PESDRn_460SX_RCEI, 0x08000000);
 
 	dcri_clrset(SDR0, port->sdr_base + PESDRn_RCSSET,
 			(PESDRx_RCSSET_RSTGU | PESDRx_RCSSET_RSTDL),
 			PESDRx_RCSSET_RSTPYN);
 
-	port->has_ibpre = 1;
 
+	/* Poll for the PHY reset */
+	while (timeout-- && !(mfdcr(port->sdr_base + PESDRn_RCSSTS) & 0x10000))
+		udelay(10);
+
+	if (!timeout)
+		return -1;
+
+	port->has_ibpre = 1;
 	return 0;
 }
 
@@ -1082,6 +1089,8 @@ static int ppc460sx_pciex_init_utl(struct ppc4xx_pciex_port *port)
 {
 	/* Max 128 Bytes */
 	out_be32 (port->utl_base + PEUTL_PBBSZ,   0x00000000);
+	out_be32 (port->utl_base + PEUTL_PCTL,	  0x80800000);
+
 	return 0;
 }
 
@@ -1308,6 +1317,7 @@ static int __init ppc4xx_pciex_port_init(struct ppc4xx_pciex_port *port)
 	 * config space accesses. That way, it will be easier to implement
 	 * hotplug later on.
 	 */
+#ifndef CONFIG_460SX
 	if (!port->has_ibpre ||
 	    !ppc4xx_pciex_wait_on_sdr(port, PESDRn_LOOP,
 				      1 << 28, 1 << 28, 100)) {
@@ -1325,6 +1335,10 @@ static int __init ppc4xx_pciex_port_init(struct ppc4xx_pciex_port *port)
 		}
 	} else
 		printk(KERN_INFO "PCIE%d: No device detected.\n", port->index);
+#else
+	/* XXX What DCR has the link status on the 460SX? */
+	port->link = 1;
+#endif
 
 	/*
 	 * Initialize mapping: disable all regions and configure
@@ -1344,8 +1358,9 @@ static int __init ppc4xx_pciex_port_init(struct ppc4xx_pciex_port *port)
 	if (ppc4xx_pciex_hwops->setup_utl)
 		ppc4xx_pciex_hwops->setup_utl(port);
 
+#ifndef CONFIG_460SX
 	/*
-	 * Check for VC0 active and assert RDY.
+	 * Check for VC0 active
 	 */
 	if (port->link &&
 	    ppc4xx_pciex_wait_on_sdr(port, PESDRn_RCSSTS,
@@ -1353,7 +1368,19 @@ static int __init ppc4xx_pciex_port_init(struct ppc4xx_pciex_port *port)
 		printk(KERN_INFO "PCIE%d: VC0 not active\n", port->index);
 		port->link = 0;
 	}
+#else
+	/*
+	 * Check for PLL Locked
+	 */
+	if (port->link &&
+	    ppc4xx_pciex_wait_on_sdr(port, PESDRn_RCSSTS,
+				     1 << 12, 1 << 12, 5000)) {
+		printk(KERN_INFO "PCIE%d: PLL not locked\n", port->index);
+		port->link = 0;
+	}
+#endif
 
+	/* Assert RDY */
 	dcri_clrset(SDR0, port->sdr_base + PESDRn_RCSSET, 0, 1 << 20);
 	msleep(100);
 
-- 
1.7.4.3

Re: [PATCH 0/1] ppc4xx: Fix PCIe scanning for the 460SX

[Benjamin Herrenschmidt]

On Fri, 2011-04-08 at 13:22 -0500, Ayman El-Khashab wrote:
> From: Ayman El-Khashab <ayman@elkhashab.com>
> 
> This patch is to fix the PCIe on the 460SX CPU.  As far as I can tell,
> the 460SX must be using a different core than the previous 4xx SOCs.
> The registers aren't the same and it appears DCRs that existed on 
> previous parts don't exist on this one.  Perhaps somebody from AMCC 
> can chime in.  In any case, the main problem was that the TX and RX
> weren't enabled so nothing downstream from the RP was ever found.
> And the check to set "port->link" would never work (as far as I could
> tell).  I could not find any DCRs in this part to indicate when the
> link was up and there isn't an obvious loopback, so the following
> call would fail and result in no devices found.
> 
> ppc4xx_pciex_wait_on_sdr(port, PESDRn_LOOP, 1 << 28, 1 << 28, 100)
> 
> It would be better to detect the link status, but I don't see an
> obvious way to do that from the data sheet.  U-boot just skips the
> link check as well for this part.

Hi Tirumala !

You originally submitted the support for 460ex. Can you chime in (and
review Ayman patch) please ?

Cheers,
Ben.

> 
> Ayman El-Khashab (1):
>   ppc4xx: Fix PCIe scanning for the 460SX
> 
>  arch/powerpc/sysdev/ppc4xx_pci.c |   35 +++++++++++++++++++++++++++++++----
>  1 files changed, 31 insertions(+), 4 deletions(-)
> 

RE: [PATCH 0/1] ppc4xx: Fix PCIe scanning for the 460SX

[Tirumala Marri]

You originally submitted the support for 460ex. Can you chime in (and
review Ayman patch) please ?

[Marri] Ben sure I will review it and send you my feedback in couple of
days.

Re: [PATCH 0/1] ppc4xx: Fix PCIe scanning for the 460SX

[Ayman El-Khashab]

On Tue, Apr 12, 2011 at 07:09:49PM -0700, Tirumala Marri wrote:
> You originally submitted the support for 460ex. Can you chime in (and
> review Ayman patch) please ?
> 
> [Marri] Ben sure I will review it and send you my feedback in couple of
> days.

Is there any update on this patch?  Any comment?  Any reason it _cant_
be included?  

Thanks
Ayman

Re: [PATCH 0/1] ppc4xx: Fix PCIe scanning for the 460SX

[Benjamin Herrenschmidt]

On Fri, 2011-04-29 at 12:02 -0500, Ayman El-Khashab wrote:
> On Tue, Apr 12, 2011 at 07:09:49PM -0700, Tirumala Marri wrote:
> > You originally submitted the support for 460ex. Can you chime in (and
> > review Ayman patch) please ?
> > 
> > [Marri] Ben sure I will review it and send you my feedback in couple of
> > days.
> 
> Is there any update on this patch?  Any comment?  Any reason it _cant_
> be included?  

I'm tempted to put it in if Tirumala doesn't get to review it asap.

Cheers,
Ben.

RE: [PATCH 0/1] ppc4xx: Fix PCIe scanning for the 460SX

[Tirumala Marri]

I'm tempted to put it in if Tirumala doesn't get to review it asap.
[Marri] Sorry for the late response. I don't see any issue with changes,
please go ahead.

Re: [PATCH 0/1] ppc4xx: Fix PCIe scanning for the 460SX

[Benjamin Herrenschmidt]

On Sat, 2011-04-30 at 06:37 +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2011-04-29 at 12:02 -0500, Ayman El-Khashab wrote:
> > On Tue, Apr 12, 2011 at 07:09:49PM -0700, Tirumala Marri wrote:
> > > You originally submitted the support for 460ex. Can you chime in (and
> > > review Ayman patch) please ?
> > > 
> > > [Marri] Ben sure I will review it and send you my feedback in couple of
> > > days.
> > 
> > Is there any update on this patch?  Any comment?  Any reason it _cant_
> > be included?  
> 
> I'm tempted to put it in if Tirumala doesn't get to review it asap.

Ok so after a closer review, the patch can't go in as-is.

The code in there is meant to work on any 44x with runtime detection of
the type of PCIe bridge.

Your patch uses #ifdef's which breaks this.

Please use the "compatible" property in the device-tree to identify the
460SX specific implementation (of_device_is_compatible(...)) and use
that to trigger your changes.

Also, the patch removes the code for waiting for the link to be up with
a comment "What DCR has the link status on the 460SX?". Please fix that
(Tirumala, can you provide the missing information ?)

Cheers,
Ben.

Re: [PATCH 0/1] ppc4xx: Fix PCIe scanning for the 460SX

[Tirumala Marri]

[-- Attachment #1: Type: text/plain, Size: 593 bytes --]

>
> Also, the patch removes the code for waiting for the link to be up with
> a comment "What DCR has the link status on the 460SX?". Please fix that
> (Tirumala, can you provide the missing information ?)
>

It is not one register. Here is the flow for Gen-1.
1. PECFGn_DLLSTA[3] will be asserted when pci-e link comes up.
2. now progream the UTL buffer configuration registers.
3. SW should assert PEUTLn_PCTL[0] to cause flow control initialization.
This is memory mapped using GPL register REGBH , REBAL and REGMSK
4. Now check for the PEUTLN_PSTA[8] for the flow control init completion.

[-- Attachment #2: Type: text/html, Size: 853 bytes --]

Re: [PATCH 0/1] ppc4xx: Fix PCIe scanning for the 460SX

[Ayman El-Khashab]

On Thu, May 05, 2011 at 09:44:27AM -0700, Tirumala Marri wrote:
> >
> > Also, the patch removes the code for waiting for the link to be up with
> > a comment "What DCR has the link status on the 460SX?". Please fix that
> > (Tirumala, can you provide the missing information ?)
> >
> 
> It is not one register. Here is the flow for Gen-1.
> 1. PECFGn_DLLSTA[3] will be asserted when pci-e link comes up.
> 2. now progream the UTL buffer configuration registers.
> 3. SW should assert PEUTLn_PCTL[0] to cause flow control initialization.
> This is memory mapped using GPL register REGBH , REBAL and REGMSK
> 4. Now check for the PEUTLN_PSTA[8] for the flow control init completion.


So what is the best way to handle this?  It appears (based
on the comments of others and my own experience) that there
is no DCR that exists and behaves the way that previous SOCs
behaved to give us the link status?  The register above 
PECFGn_DLLSTA is actually in the PCIe configuration space so
we would have to map that in to be able to read that
register during the link check.  Is that correct or ok?

I've communicated with some people over email and they had
tried the (PESDRn_HSSLySTS) register.  Recognizing that
there exists one of these for each port/lane, is there a way
to use this one?  It is in the indirect DCR space.  I'd
tried this myself and never did get it to do anything but I
could have been looking at the wrong lane or something.

Lastly, what was the reason for forcing the original code to
be GEN-1 speeds?

ayman

RE: [PATCH 0/1] ppc4xx: Fix PCIe scanning for the 460SX

[Tirumala Marri]

So what is the best way to handle this?  It appears (based
on the comments of others and my own experience) that there
is no DCR that exists and behaves the way that previous SOCs
behaved to give us the link status?  The register above
PECFGn_DLLSTA is actually in the PCIe configuration space so
we would have to map that in to be able to read that
register during the link check.  Is that correct or ok?
[marri] yes, you need to program DCR register access these local PCIE_CFG
registers.


I've communicated with some people over email and they had
tried the (PESDRn_HSSLySTS) register.  Recognizing that
there exists one of these for each port/lane, is there a way
to use this one?  It is in the indirect DCR space.  I'd
tried this myself and never did get it to do anything but I
could have been looking at the wrong lane or something.
[marri]This is at SERDES level. If this link up doesn't necessarily
Overall stack is up. This is mostly used for BIST and diagnostics.

Lastly, what was the reason for forcing the original code to
be GEN-1 speeds?
[marri] Gen-2 need some extra checks compared to Gen-1.
There were not many Gen-2 devices at the time of submission
To test them.

RE: [PATCH 0/1] ppc4xx: Fix PCIe scanning for the 460SX

[Benjamin Herrenschmidt]

On Thu, 2011-05-12 at 11:16 -0700, Tirumala Marri wrote:
> So what is the best way to handle this?  It appears (based
> on the comments of others and my own experience) that there
> is no DCR that exists and behaves the way that previous SOCs
> behaved to give us the link status? 

 The register above
> PECFGn_DLLSTA is actually in the PCIe configuration space so
> we would have to map that in to be able to read that
> register during the link check.  Is that correct or ok?
> [marri] yes, you need to program DCR register access these local PCIE_CFG
> registers.

We do I think, tho we might have to re-order some stuff. I'm facing a
similar issue with some internal design here, I'm thinking off adding a
new hook to the ppc4xx_pciex_hwops for link checking to replace the SDR
business.

The interesting question of course is whether that 460SX stuff is the
same as what we're using internally :-)

> I've communicated with some people over email and they had
> tried the (PESDRn_HSSLySTS) register.  Recognizing that
> there exists one of these for each port/lane, is there a way
> to use this one?  It is in the indirect DCR space.  I'd
> tried this myself and never did get it to do anything but I
> could have been looking at the wrong lane or something.
> [marri]This is at SERDES level. If this link up doesn't necessarily
> Overall stack is up. This is mostly used for BIST and diagnostics.
> 
> Lastly, what was the reason for forcing the original code to
> be GEN-1 speeds?
> [marri] Gen-2 need some extra checks compared to Gen-1.
> There were not many Gen-2 devices at the time of submission
> To test them.

Can we fix that ?

Cheers,
Ben.

Re: [PATCH 0/1] ppc4xx: Fix PCIe scanning for the 460SX

[Ayman El-Khashab]

On Tue, May 24, 2011 at 01:40:12PM +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2011-05-12 at 11:16 -0700, Tirumala Marri wrote:
> 
>  The register above
> > PECFGn_DLLSTA is actually in the PCIe configuration space so
> > we would have to map that in to be able to read that
> > register during the link check.  Is that correct or ok?
> > [marri] yes, you need to program DCR register access these local PCIE_CFG
> > registers.
> 
> We do I think, tho we might have to re-order some stuff. I'm facing a
> similar issue with some internal design here, I'm thinking off adding a
> new hook to the ppc4xx_pciex_hwops for link checking to replace the SDR
> business.

That would probably solve the existing problem.  I might be
remembering it wrong (or reading it wrong) or both.  But I
thought that the config space was mapped in the setup_hose
that happens after (and if) the link check finished
successfully.  

> The interesting question of course is whether that 460SX stuff is the
> same as what we're using internally :-)

Not sure how I would know --  But with my eiger kit, I got a
cd from amcc that had a patched 2.6.30 or something kernel
in it to support the 460SX.  The pci code was basically
subverted by adding a "port->link=1" at the very end of the
link check to always force it to succeed.  However this code
never appeared in the public git repositories, so I am not
sure how the 460SX functioned (if it ever did) since the
link check that is in there now can't work on that SOC.

> > Lastly, what was the reason for forcing the original code to
> > be GEN-1 speeds?
> > [marri] Gen-2 need some extra checks compared to Gen-1.
> > There were not many Gen-2 devices at the time of submission
> > To test them.
> 
> Can we fix that ?
> 

I took care of that in my patch.  Basically it let the
system go to gen-2 speeds and negotiate down.

Ayman

Re: [PATCH 0/1] ppc4xx: Fix PCIe scanning for the 460SX

[Benjamin Herrenschmidt]

On Fri, 2011-05-27 at 11:51 -0500, Ayman El-Khashab wrote:
> On Tue, May 24, 2011 at 01:40:12PM +1000, Benjamin Herrenschmidt wrote:
> > On Thu, 2011-05-12 at 11:16 -0700, Tirumala Marri wrote:
> > 
> >  The register above
> > > PECFGn_DLLSTA is actually in the PCIe configuration space so
> > > we would have to map that in to be able to read that
> > > register during the link check.  Is that correct or ok?
> > > [marri] yes, you need to program DCR register access these local PCIE_CFG
> > > registers.
> > 
> > We do I think, tho we might have to re-order some stuff. I'm facing a
> > similar issue with some internal design here, I'm thinking off adding a
> > new hook to the ppc4xx_pciex_hwops for link checking to replace the SDR
> > business.
> 
> That would probably solve the existing problem.  I might be
> remembering it wrong (or reading it wrong) or both.  But I
> thought that the config space was mapped in the setup_hose
> that happens after (and if) the link check finished
> successfully.  

Yes but we already map separately the host part and the generic part of
cfg, so it wouldn't be hard to move one of these around.

> > The interesting question of course is whether that 460SX stuff is the
> > same as what we're using internally :-)
> 
> Not sure how I would know --  But with my eiger kit, I got a
> cd from amcc that had a patched 2.6.30 or something kernel
> in it to support the 460SX.  The pci code was basically
> subverted by adding a "port->link=1" at the very end of the
> link check to always force it to succeed.  

Niiiiiiice .... not. And they didn't even bothered to submit
anything :-(

> However this code
> never appeared in the public git repositories, so I am not
> sure how the 460SX functioned (if it ever did) since the
> link check that is in there now can't work on that SOC.

Yeah odd. Looks like MyAMCC account is no longer working so I can't get
to the docs, at least until I create a new one and get it re-authorized
I suppose.

Anyways, I'll come up with something (or rather Tony Breeds will) some
time next week that does this refactoring and we'll see what's up from
there.

> > > Lastly, what was the reason for forcing the original code to
> > > be GEN-1 speeds?
> > > [marri] Gen-2 need some extra checks compared to Gen-1.
> > > There were not many Gen-2 devices at the time of submission
> > > To test them.
> > 
> > Can we fix that ?
> > 
> 
> I took care of that in my patch.  Basically it let the
> system go to gen-2 speeds and negotiate down.

Ah ok good.

Cheers,
Ben.

RE: [PATCH 0/1] ppc4xx: Fix PCIe scanning for the 460SX

[Tirumala Marri]

The interesting question of course is whether that 460SX stuff is the
same as what we're using internally :-)
[marri] Sometimes open-source and internal releases may not be the same
Because of open-source standards.

Can we fix that ?
[marri] Sure I will take a look at it.

RE: [PATCH 0/1] ppc4xx: Fix PCIe scanning for the 460SX

[Tirumala Marri]

Not sure how I would know --  But with my eiger kit, I got a
cd from amcc that had a patched 2.6.30 or something kernel
in it to support the 460SX.  The pci code was basically
subverted by adding a "port->link=1" at the very end of the
link check to always force it to succeed.  However this code
never appeared in the public git repositories, so I am not
sure how the 460SX functioned (if it ever did) since the
link check that is in there now can't work on that SOC.


[marri]I don't know what PCI-E devices you use. We use E1000
As a device for testing. Link-up was working before the submission.
There were some changes happened afterwards to the common code
Which seems to affected all 4xx devices. I will try latest code
On our other SoCs.


I took care of that in my patch.  Basically it let the
system go to gen-2 speeds and negotiate down.
[marri] Great thx.

Re: [PATCH 0/1] ppc4xx: Fix PCIe scanning for the 460SX

[Ayman El-Khashab]

On Tue, May 31, 2011 at 02:27:19PM -0700, Tirumala Marri wrote:
> Not sure how I would know --  But with my eiger kit, I got a
> cd from amcc that had a patched 2.6.30 or something kernel
> in it to support the 460SX.  The pci code was basically
> subverted by adding a "port->link=1" at the very end of the
> link check to always force it to succeed.  However this code
> never appeared in the public git repositories, so I am not
> sure how the 460SX functioned (if it ever did) since the
> link check that is in there now can't work on that SOC.
> 
> 
> [marri]I don't know what PCI-E devices you use. We use E1000
> As a device for testing. Link-up was working before the submission.
> There were some changes happened afterwards to the common code
> Which seems to affected all 4xx devices. I will try latest code
> On our other SoCs.

Well, briefly we build a custom processor board that has
a couple powerpcs on it and some pcie switches.  We end up
plugging our devices into the switches which is unlike what
most people try.  In any case, we use several different
drive controllers for SAS, SCSI, and SSDs.  For testing
purposes (i.e. when things don't go the way they should) we
have an adapter to make "regular" pcie slots that we plug in
whatever.  But our switches are on the board with the CPUs
so as far as the CPU goes, it will always see a gen-2 link
8 lane link, irrespective of what the endpoint actually is.
We let the switches handle the speed changes and the port
bifurcation.  

> I took care of that in my patch.  Basically it let the
> system go to gen-2 speeds and negotiate down.
> [marri] Great thx.

Ok, so I am back from doing whatever it is that I do.  Shall
I go ahead and take a stab at making a new functor to handle
the "link check"? -- which IIRC was the thought from last
month about how best to handle the 460SX and whatever some
people were using internally.

Ayman

Re: [PATCH 0/1] ppc4xx: Fix PCIe scanning for the 460SX

[Benjamin Herrenschmidt]

On Mon, 2011-06-27 at 05:14 -0500, Ayman El-Khashab wrote:
> > I took care of that in my patch.  Basically it let the
> > system go to gen-2 speeds and negotiate down.
> > [marri] Great thx.
> 
> Ok, so I am back from doing whatever it is that I do.  Shall
> I go ahead and take a stab at making a new functor to handle
> the "link check"? -- which IIRC was the thought from last
> month about how best to handle the 460SX and whatever some
> people were using internally. 

Catch up with Tony (CC), he's working on something similar for a new
variant of that PCIe controller used internally by some IBM stuff.

Cheers,
Ben.