RE: [PATCH 3/3] powerpc/44x: Add support PCI-E for APM821xx SoC and Bluestone board

Hi Josh,
Please see below for my comments. If you have any concerns or suggestion,
please let me know.
Best regards,
Vinh Nguyen.

-----Original Message-----
From: Josh Boyer [mailto:jwboyer@gmail.com]
Sent: Tuesday, December 20, 2011 10:32 PM
To: Vinh Nguyen Huu Tuong
Cc: Benjamin Herrenschmidt; Paul Mackerras; Matt Porter; Kumar Gala; Paul
Gortmaker; Anton Blanchard; Dave Kleikamp; Grant Likely; Tony Breeds; Rob
Herring; Jiri Kosina; Lucas De Marchi; Ayman El-Khashab;
linuxppc-dev@lists.ozlabs.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/3] powerpc/44x: Add support PCI-E for APM821xx SoC
and Bluestone board

On Tue, Dec 20, 2011 at 7:44 AM, Vinh Nguyen Huu Tuong
[off-list ref] wrote:

This patch extends PCI-E driver to support PCI-E for APM821xx SoC on

Bluestone board.

Signed-off-by: Vinh Nguyen Huu Tuong <redacted>

+static int apm821xx_pciex_init_port_hw(struct ppc4xx_pciex_port *port)
+{
+ =A0 =A0 =A0 u32 val;
+ =A0 =A0 =A0 u32 utlset1;
+ =A0 =A0 =A0 u32 timeout;
+
+ =A0 =A0 =A0 /*
+ =A0 =A0 =A0 =A0* Do a software reset on PCIe ports.
+ =A0 =A0 =A0 =A0* This code is to fix the issue that pci drivers doesn't

re-assign

+ =A0 =A0 =A0 =A0* bus number for PCIE devices after Uboot
+ =A0 =A0 =A0 =A0* scanned and configured all the buses (eg. PCIE NIC

IntelPro/1000

+ =A0 =A0 =A0 =A0* PT quad port, SAS LSI 1064E)
+ =A0 =A0 =A0 =A0*/
+
+ =A0 =A0 =A0 mtdcri(SDR0, PESDR0_460EX_PHY_CTL_RST + (port->index * 0x55=

),
0x0);

+ =A0 =A0 =A0 mdelay(10);
+
+ =A0 =A0 =A0 if (port->endpoint)
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 val =3D PTYPE_LEGACY_ENDPOINT << 20;
+ =A0 =A0 =A0 else
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 val =3D PTYPE_ROOT_PORT << 20;
+
+ =A0 =A0 =A0 if (port->index =3D=3D 0) {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 val |=3D LNKW_X1 << 12;
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 utlset1 =3D 0x00000000;
+ =A0 =A0 =A0 } else {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 val |=3D LNKW_X4 << 12;
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 utlset1 =3D 0x20101101;
+ =A0 =A0 =A0 }
+
+ =A0 =A0 =A0 mtdcri(SDR0, port->sdr_base + PESDRn_DLPSET, val);
+ =A0 =A0 =A0 mtdcri(SDR0, port->sdr_base + PESDRn_UTLSET1, utlset1);
+ =A0 =A0 =A0 mtdcri(SDR0, port->sdr_base + PESDRn_UTLSET2, 0x01010000);
+
+ =A0 =A0 =A0 switch (port->index) {
+ =A0 =A0 =A0 case 0:
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mtdcri(SDR0, PESDR0_460EX_L0CDRCTL, 0x00003=

230);

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mtdcri(SDR0, PESDR0_460EX_L0DRV, 0x00000130=

);

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mtdcri(SDR0, PESDR0_460EX_L0CLK, 0x00000006=

);

+
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mtdcri(SDR0, PESDR0_460EX_PHY_CTL_RST, 0x10=

000000);

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mdelay(50);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mtdcri(SDR0, PESDR0_460EX_PHY_CTL_RST, 0x30=

000000);

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
+
+ =A0 =A0 =A0 case 1:
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mtdcri(SDR0, PESDR1_460EX_L0CDRCTL, 0x00003=

230);

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mtdcri(SDR0, PESDR1_460EX_L1CDRCTL, 0x00003=

230);

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mtdcri(SDR0, PESDR1_460EX_L2CDRCTL, 0x00003=

230);

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mtdcri(SDR0, PESDR1_460EX_L3CDRCTL, 0x00003=

230);

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mtdcri(SDR0, PESDR1_460EX_L0DRV, 0x00000130=

);

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mtdcri(SDR0, PESDR1_460EX_L1DRV, 0x00000130=

);

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mtdcri(SDR0, PESDR1_460EX_L2DRV, 0x00000130=

);

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mtdcri(SDR0, PESDR1_460EX_L3DRV, 0x00000130=

);

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mtdcri(SDR0, PESDR1_460EX_L0CLK, 0x00000006=

);

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mtdcri(SDR0, PESDR1_460EX_L1CLK, 0x00000006=

);

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mtdcri(SDR0, PESDR1_460EX_L2CLK, 0x00000006=

);

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mtdcri(SDR0, PESDR1_460EX_L3CLK, 0x00000006=

);

+
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mtdcri(SDR0, PESDR1_460EX_PHY_CTL_RST, 0x10=

000000);

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
+ =A0 =A0 =A0 }

Do we need a default case here to catch oddness and exit the function?
[Vinh Nguyen] we've already checked the port->index before calling this
function, so we assume that the port->index never exceed the checked
values.

+
+ =A0 =A0 =A0 mtdcri(SDR0, port->sdr_base + PESDRn_RCSSET,
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mfdcri(SDR0, port->sdr_base + PESDRn_RCSSET=

) |

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 (PESDRx_RCSSET_RSTGU | PESDRx_RCSSET_RSTPYN=

));

+
+ =A0 =A0 =A0 /* Poll for PHY reset */
+ =A0 =A0 =A0 timeout =3D 0;
+ =A0 =A0 =A0 while ((!(mfdcri(SDR0, PESDR0_460EX_RSTSTA +
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (port->index * 0x55)) & 0x1=

)) &&

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(timeout < PCIE_PHY_RESET_TIMEOUT)) {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(10);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 timeout++;
+ =A0 =A0 =A0 }
+
+ =A0 =A0 =A0 if (timeout < PCIE_PHY_RESET_TIMEOUT) {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 mtdcri(SDR0, port->sdr_base + PESDRn_RCSSET=

,

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (mfdcri(SDR0, port->sdr_bas=

e + PESDRn_RCSSET) &

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ~(PESDRx_RCSSET_RSTGU | PES=

DRx_RCSSET_RSTDL)) |

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 PESDRx_RCSSET_RSTPYN);
+
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 port->has_ibpre =3D 1;
+
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 return 0;
+ =A0 =A0 =A0 } else {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN_INFO "PCIE: Can't reset PHY\n")=

;

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -1;
+ =A0 =A0 =A0 }

If we can't reset the PHY, does this whole function essentially fail?
Do the devices not get renumbered, etc?  If so, you probably want to
make that KERN_ERR.
[Vinh Nguyen] if we can't reset the PHY, maybe the device can't work
properly. I will update codes to return the error in case PHY can't reset.

ppc4xx_configure_pciex_PIMs(struct ppc4xx_pciex_port *port,

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 * if it works
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 */
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0out_le32(mbase + PECFG_PIM0LAL, 0x00000000=

);

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 out_le32(mbase + PECFG_PIM0LAH, 0x00000000)=

;

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 out_le32(mbase + PECFG_PIM0LAH, 0x00000008)=

; /* Moving
on HB */

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0out_le32(mbase + PECFG_PIM1LAL, 0x00000000=

);

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 out_le32(mbase + PECFG_PIM1LAH, 0x00000000)=

;

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 out_le32(mbase + PECFG_PIM1LAH, 0x0000000c)=

; /* Moving
on HB */

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0out_le32(mbase + PECFG_PIM01SAH, 0xffff000=

0);

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0out_le32(mbase + PECFG_PIM01SAL, 0x0000000=

0);

Why are these values changed, and are those changes only needed on
APM821xx?
[Vinh Nguyen] In system memory map of 460EX chip, we have an "alias DDR
SDRAM" area that is Local Memory Alias for HB(high bandwidth), so we tried
to initialize it for speedup. For APM821xx, although we didn't mention
about this area, this configuration still works well. So we keep it.

b/arch/powerpc/sysdev/ppc4xx_pci.h

Is this value applicable to all the 44x devices with PCI-e?
[Vinh Nguyen] At this time, we only checked this on APM821xx chips.
josh