| Message ID | 1310603611-8960-2-git-send-email-ayman@elkhashab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Jul 13, 2011 at 07:33:31PM -0500, Ayman El-Khashab wrote: > Adds a register to the config space for the 460sx. Changes the vc0 > detect to a pll detect. maps configuration space to test the link > status. changes the setup to enable gen2 devices to operate at gen2 > speeds. fixes mapping that was not correct for the 460sx. > > tested on the 460sx eiger and custom board Hi Ayman. Just a few comments. <snip> > +static void __init ppc460sx_pciex_check_link(struct ppc4xx_pciex_port *port) > +{ > + void __iomem *mbase; > + int attempt = 50; > + > + port->link = 0; > + > + mbase = ioremap(port->cfg_space.start + 0x00000000, 0x1000); Why + 0x00000000 ? ppc4xx_pciex_port_setup_hose() does: mbase = ioremap(port->cfg_space.start + 0x10000000, 0x1000); so isn't one of these statements is wrong? > + if (mbase == NULL) { > + printk(KERN_ERR "%s: Can't map internal config space !", > + port->node->full_name); > + goto done; > + } > + > + while (attempt && (0 == (in_le32(mbase + PECFG_460SX_DLLSTA) > + & 0x00000010))) { Nitpicking, I think it'd be nice if there was #define for 0x00000010 perhaps: #define PECFG_460SX_DLLSTA_LINKUP 0x00000010 > + attempt--; > + mdelay(10); > + } > + if (attempt) > + port->link = 1; > +done: > + iounmap(mbase); > + > +} > + > static struct ppc4xx_pciex_hwops ppc460sx_pcie_hwops __initdata = { > .core_init = ppc460sx_pciex_core_init, > .port_init_hw = ppc460sx_pciex_init_port_hw, > .setup_utl = ppc460sx_pciex_init_utl, > - .check_link = ppc4xx_pciex_check_link_sdr, > + .check_link = ppc460sx_pciex_check_link, > }; > > #endif /* CONFIG_44x */ > @@ -1338,15 +1367,15 @@ static int __init ppc4xx_pciex_port_init(struct ppc4xx_pciex_port *port) > if (rc != 0) > return rc; > > - if (ppc4xx_pciex_hwops->check_link) > - ppc4xx_pciex_hwops->check_link(port); > - > /* > * Initialize mapping: disable all regions and configure > * CFG and REG regions based on resources in the device tree > */ > ppc4xx_pciex_port_init_mapping(port); > > + if (ppc4xx_pciex_hwops->check_link) > + ppc4xx_pciex_hwops->check_link(port); > + Why move this? You already iorempat the cfg space. > /* > * Map UTL > */ > @@ -1360,13 +1389,23 @@ static int __init ppc4xx_pciex_port_init(struct ppc4xx_pciex_port *port) > ppc4xx_pciex_hwops->setup_utl(port); > > /* > - * Check for VC0 active and assert RDY. > + * Check for VC0 active or PLL Locked and assert RDY. > */ > if (port->sdr_base) { > - if (port->link && > - ppc4xx_pciex_wait_on_sdr(port, PESDRn_RCSSTS, > - 1 << 16, 1 << 16, 5000)) { > - printk(KERN_INFO "PCIE%d: VC0 not active\n", port->index); > + if (of_device_is_compatible(port->node, > + "ibm,plb-pciex-460sx")){ > + 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; > + } > + } else if (port->link && > + ppc4xx_pciex_wait_on_sdr(port, PESDRn_RCSSTS, > + 1 << 16, 1 << 16, 5000)) { > + printk(KERN_INFO "PCIE%d: VC0 not active\n", > + port->index); > port->link = 0; > } > > @@ -1565,6 +1604,10 @@ static int __init ppc4xx_setup_one_pciex_POM(struct ppc4xx_pciex_port *port, > pcial = RES_TO_U32_LOW(pci_addr); > sa = (0xffffffffu << ilog2(size)) | 0x1; > > + /* Enabled and single region */ > + sa |= (of_device_is_compatible(port->node, "ibm,plb-pciex-460sx")) ? > + 0x5 : 0x3; > + > /* Program register values */ > switch (index) { > case 0: > @@ -1573,8 +1616,7 @@ static int __init ppc4xx_setup_one_pciex_POM(struct ppc4xx_pciex_port *port, > dcr_write(port->dcrs, DCRO_PEGPL_OMR1BAH, lah); > dcr_write(port->dcrs, DCRO_PEGPL_OMR1BAL, lal); > dcr_write(port->dcrs, DCRO_PEGPL_OMR1MSKH, 0x7fffffff); > - /* Note that 3 here means enabled | single region */ > - dcr_write(port->dcrs, DCRO_PEGPL_OMR1MSKL, sa | 3); > + dcr_write(port->dcrs, DCRO_PEGPL_OMR1MSKL, sa); > break; > case 1: > out_le32(mbase + PECFG_POM1LAH, pciah); > @@ -1582,8 +1624,7 @@ static int __init ppc4xx_setup_one_pciex_POM(struct ppc4xx_pciex_port *port, > dcr_write(port->dcrs, DCRO_PEGPL_OMR2BAH, lah); > dcr_write(port->dcrs, DCRO_PEGPL_OMR2BAL, lal); > dcr_write(port->dcrs, DCRO_PEGPL_OMR2MSKH, 0x7fffffff); > - /* Note that 3 here means enabled | single region */ > - dcr_write(port->dcrs, DCRO_PEGPL_OMR2MSKL, sa | 3); > + dcr_write(port->dcrs, DCRO_PEGPL_OMR2MSKL, sa); > break; > case 2: > out_le32(mbase + PECFG_POM2LAH, pciah); > @@ -1591,8 +1632,7 @@ static int __init ppc4xx_setup_one_pciex_POM(struct ppc4xx_pciex_port *port, > dcr_write(port->dcrs, DCRO_PEGPL_OMR3BAH, lah); > dcr_write(port->dcrs, DCRO_PEGPL_OMR3BAL, lal); > dcr_write(port->dcrs, DCRO_PEGPL_OMR3MSKH, 0x7fffffff); > - /* Note that 3 here means enabled | IO space !!! */ > - dcr_write(port->dcrs, DCRO_PEGPL_OMR3MSKL, sa | 3); > + dcr_write(port->dcrs, DCRO_PEGPL_OMR3MSKL, sa); > break; > } I think you really want to check the definitions for OMRs 2 and 3 to verify that this is right. Yours Tony
Thanks Tony, some comments below. On Thu, Jul 14, 2011 at 11:16:27AM +1000, Tony Breeds wrote: > > > +static void __init ppc460sx_pciex_check_link(struct ppc4xx_pciex_port *port) > > +{ > > + void __iomem *mbase; > > + int attempt = 50; > > + > > + port->link = 0; > > + > > + mbase = ioremap(port->cfg_space.start + 0x00000000, 0x1000); > > Why + 0x00000000 ? ppc4xx_pciex_port_setup_hose() does: > mbase = ioremap(port->cfg_space.start + 0x10000000, 0x1000); > so isn't one of these statements is wrong? yes, that doesn't look right. I'll verify that and make sure that it works correctly and resubmit the patch. > > > + if (mbase == NULL) { > > + printk(KERN_ERR "%s: Can't map internal config space !", > > + port->node->full_name); > > + goto done; > > + } > > + > > + while (attempt && (0 == (in_le32(mbase + PECFG_460SX_DLLSTA) > > + & 0x00000010))) { > > Nitpicking, I think it'd be nice if there was #define for 0x00000010 > perhaps: #define PECFG_460SX_DLLSTA_LINKUP 0x00000010 ok. > > > > - if (ppc4xx_pciex_hwops->check_link) > > - ppc4xx_pciex_hwops->check_link(port); > > - > > /* > > * Initialize mapping: disable all regions and configure > > * CFG and REG regions based on resources in the device tree > > */ > > ppc4xx_pciex_port_init_mapping(port); > > > > + if (ppc4xx_pciex_hwops->check_link) > > + ppc4xx_pciex_hwops->check_link(port); > > + > > Why move this? You already iorempat the cfg space. This was what I was asking about before. The reason that I swapped the order of the init_mapping and check_link is because the init_mapping currently sets up the cfgbax registers. Those setup the base address of the configuration space on the PLB side of the bus. As far as I could determine, I cannot access the config space until those registers are configured. I need to touch the config space in order to do the check_link b/c the 460sx uses the extended config space to keep track of the link status. I looked at init mapping and based on what it did I did not see any potential adverse effects. > > out_le32(mbase + PECFG_POM2LAH, pciah); > > @@ -1591,8 +1632,7 @@ static int __init ppc4xx_setup_one_pciex_POM(struct ppc4xx_pciex_port *port, > > dcr_write(port->dcrs, DCRO_PEGPL_OMR3BAH, lah); > > dcr_write(port->dcrs, DCRO_PEGPL_OMR3BAL, lal); > > dcr_write(port->dcrs, DCRO_PEGPL_OMR3MSKH, 0x7fffffff); > > - /* Note that 3 here means enabled | IO space !!! */ > > - dcr_write(port->dcrs, DCRO_PEGPL_OMR3MSKL, sa | 3); > > + dcr_write(port->dcrs, DCRO_PEGPL_OMR3MSKL, sa); > > break; > > } > > I think you really want to check the definitions for OMRs 2 and 3 to verify that this is right. Thanks, good catch. I'll change the first case block to include a switch on the 460sx. The first case statement needs to be | 0x5, while the others need to stay 0x3. Ayman
On Thu, 2011-07-14 at 11:04 -0500, Ayman El-Khashab wrote: > Thanks Tony, some comments below. > > On Thu, Jul 14, 2011 at 11:16:27AM +1000, Tony Breeds wrote: > > > > > +static void __init ppc460sx_pciex_check_link(struct ppc4xx_pciex_port *port) > > > +{ > > > + void __iomem *mbase; > > > + int attempt = 50; > > > + > > > + port->link = 0; > > > + > > > + mbase = ioremap(port->cfg_space.start + 0x00000000, 0x1000); > > > > Why + 0x00000000 ? ppc4xx_pciex_port_setup_hose() does: > > mbase = ioremap(port->cfg_space.start + 0x10000000, 0x1000); > > so isn't one of these statements is wrong? > > yes, that doesn't look right. I'll verify that and make > sure that it works correctly and resubmit the patch. The state of that top bit is obscure. I couldn't figure out from docs whether it's actually useful or not (it doesn't seem to make a difference on the HW we've played with here). It's possible that some parts need it to access the RC config space vs emitting type 1 cycles, and some don't, in which case the address decoding just wraps and the bit is ignored ? > > > + if (mbase == NULL) { > > > + printk(KERN_ERR "%s: Can't map internal config space !", > > > + port->node->full_name); > > > + goto done; > > > + } > > > + > > > + while (attempt && (0 == (in_le32(mbase + PECFG_460SX_DLLSTA) > > > + & 0x00000010))) { > > > > Nitpicking, I think it'd be nice if there was #define for 0x00000010 > > perhaps: #define PECFG_460SX_DLLSTA_LINKUP 0x00000010 > > ok. > > > > > > > - if (ppc4xx_pciex_hwops->check_link) > > > - ppc4xx_pciex_hwops->check_link(port); > > > - > > > /* > > > * Initialize mapping: disable all regions and configure > > > * CFG and REG regions based on resources in the device tree > > > */ > > > ppc4xx_pciex_port_init_mapping(port); > > > > > > + if (ppc4xx_pciex_hwops->check_link) > > > + ppc4xx_pciex_hwops->check_link(port); > > > + > > > > Why move this? You already iorempat the cfg space. > > This was what I was asking about before. The reason that I > swapped the order of the init_mapping and check_link is > because the init_mapping currently sets up the cfgbax > registers. Those setup the base address of the > configuration space on the PLB side of the bus. As far as I > could determine, I cannot access the config space until > those registers are configured. I need to touch the config > space in order to do the check_link b/c the 460sx uses the > extended config space to keep track of the link status. I > looked at init mapping and based on what it did I did not > see any potential adverse effects. I think it makes sense to setup mappings before checking the link. There may be a couple of mechanical issues in the code with this, I haven't looked, but I agree in principle. > > > out_le32(mbase + PECFG_POM2LAH, pciah); > > > @@ -1591,8 +1632,7 @@ static int __init ppc4xx_setup_one_pciex_POM(struct ppc4xx_pciex_port *port, > > > dcr_write(port->dcrs, DCRO_PEGPL_OMR3BAH, lah); > > > dcr_write(port->dcrs, DCRO_PEGPL_OMR3BAL, lal); > > > dcr_write(port->dcrs, DCRO_PEGPL_OMR3MSKH, 0x7fffffff); > > > - /* Note that 3 here means enabled | IO space !!! */ > > > - dcr_write(port->dcrs, DCRO_PEGPL_OMR3MSKL, sa | 3); > > > + dcr_write(port->dcrs, DCRO_PEGPL_OMR3MSKL, sa); > > > break; > > > } > > > > I think you really want to check the definitions for OMRs 2 and 3 to verify that this is right. > > Thanks, good catch. I'll change the first case block to > include a switch on the 460sx. The first case statement > needs to be | 0x5, while the others need to stay 0x3. Please make it constants using #define or something... those bits tend to subtly change between ASICs too I noticed. Cheers, Ben. > Ayman > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
From: Ayman El-Khashab <ayman@elkhashab.com>
Changes from v1->v2
Added definitions for the bits in the OMRxMSKL registers
Refactored the setting of the OMRxMSKL registers for the 460SX
Added bit defines for the PECFG_460SX_DLLSTA register
Changes from v2->v3
Fixed commit message to be more clear as to what was done
Ayman El-Khashab (1):
powerpc/4xx: enable and fix pcie gen1/gen2 on the 460sx
arch/powerpc/sysdev/ppc4xx_pci.c | 89 ++++++++++++++++++++++++++++++-------
arch/powerpc/sysdev/ppc4xx_pci.h | 12 +++++
2 files changed, 84 insertions(+), 17 deletions(-)
diff --git a/arch/powerpc/sysdev/ppc4xx_pci.c b/arch/powerpc/sysdev/ppc4xx_pci.c index ad330fe..273963b 100644 --- a/arch/powerpc/sysdev/ppc4xx_pci.c +++ b/arch/powerpc/sysdev/ppc4xx_pci.c @@ -1092,6 +1092,10 @@ static int __init ppc460sx_pciex_core_init(struct device_node *np) mtdcri(SDR0, PESDR1_460SX_HSSSLEW, 0xFFFF0000); mtdcri(SDR0, PESDR2_460SX_HSSSLEW, 0xFFFF0000); + /* Set HSS PRBS enabled */ + mtdcri(SDR0, PESDR0_460SX_HSSCTLSET, 0x00001130); + mtdcri(SDR0, PESDR2_460SX_HSSCTLSET, 0x00001130); + udelay(100); /* De-assert PLLRESET */ @@ -1132,9 +1136,6 @@ 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); @@ -1148,14 +1149,42 @@ static int ppc460sx_pciex_init_utl(struct ppc4xx_pciex_port *port) { /* Max 128 Bytes */ out_be32 (port->utl_base + PEUTL_PBBSZ, 0x00000000); + /* Assert VRB and TXE - per datasheet turn off addr validation */ + out_be32(port->utl_base + PEUTL_PCTL, 0x80800000); return 0; } +static void __init ppc460sx_pciex_check_link(struct ppc4xx_pciex_port *port) +{ + void __iomem *mbase; + int attempt = 50; + + port->link = 0; + + mbase = ioremap(port->cfg_space.start + 0x00000000, 0x1000); + if (mbase == NULL) { + printk(KERN_ERR "%s: Can't map internal config space !", + port->node->full_name); + goto done; + } + + while (attempt && (0 == (in_le32(mbase + PECFG_460SX_DLLSTA) + & 0x00000010))) { + attempt--; + mdelay(10); + } + if (attempt) + port->link = 1; +done: + iounmap(mbase); + +} + static struct ppc4xx_pciex_hwops ppc460sx_pcie_hwops __initdata = { .core_init = ppc460sx_pciex_core_init, .port_init_hw = ppc460sx_pciex_init_port_hw, .setup_utl = ppc460sx_pciex_init_utl, - .check_link = ppc4xx_pciex_check_link_sdr, + .check_link = ppc460sx_pciex_check_link, }; #endif /* CONFIG_44x */ @@ -1338,15 +1367,15 @@ static int __init ppc4xx_pciex_port_init(struct ppc4xx_pciex_port *port) if (rc != 0) return rc; - if (ppc4xx_pciex_hwops->check_link) - ppc4xx_pciex_hwops->check_link(port); - /* * Initialize mapping: disable all regions and configure * CFG and REG regions based on resources in the device tree */ ppc4xx_pciex_port_init_mapping(port); + if (ppc4xx_pciex_hwops->check_link) + ppc4xx_pciex_hwops->check_link(port); + /* * Map UTL */ @@ -1360,13 +1389,23 @@ static int __init ppc4xx_pciex_port_init(struct ppc4xx_pciex_port *port) ppc4xx_pciex_hwops->setup_utl(port); /* - * Check for VC0 active and assert RDY. + * Check for VC0 active or PLL Locked and assert RDY. */ if (port->sdr_base) { - if (port->link && - ppc4xx_pciex_wait_on_sdr(port, PESDRn_RCSSTS, - 1 << 16, 1 << 16, 5000)) { - printk(KERN_INFO "PCIE%d: VC0 not active\n", port->index); + if (of_device_is_compatible(port->node, + "ibm,plb-pciex-460sx")){ + 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; + } + } else if (port->link && + ppc4xx_pciex_wait_on_sdr(port, PESDRn_RCSSTS, + 1 << 16, 1 << 16, 5000)) { + printk(KERN_INFO "PCIE%d: VC0 not active\n", + port->index); port->link = 0; } @@ -1565,6 +1604,10 @@ static int __init ppc4xx_setup_one_pciex_POM(struct ppc4xx_pciex_port *port, pcial = RES_TO_U32_LOW(pci_addr); sa = (0xffffffffu << ilog2(size)) | 0x1; + /* Enabled and single region */ + sa |= (of_device_is_compatible(port->node, "ibm,plb-pciex-460sx")) ? + 0x5 : 0x3; + /* Program register values */ switch (index) { case 0: @@ -1573,8 +1616,7 @@ static int __init ppc4xx_setup_one_pciex_POM(struct ppc4xx_pciex_port *port, dcr_write(port->dcrs, DCRO_PEGPL_OMR1BAH, lah); dcr_write(port->dcrs, DCRO_PEGPL_OMR1BAL, lal); dcr_write(port->dcrs, DCRO_PEGPL_OMR1MSKH, 0x7fffffff); - /* Note that 3 here means enabled | single region */ - dcr_write(port->dcrs, DCRO_PEGPL_OMR1MSKL, sa | 3); + dcr_write(port->dcrs, DCRO_PEGPL_OMR1MSKL, sa); break; case 1: out_le32(mbase + PECFG_POM1LAH, pciah); @@ -1582,8 +1624,7 @@ static int __init ppc4xx_setup_one_pciex_POM(struct ppc4xx_pciex_port *port, dcr_write(port->dcrs, DCRO_PEGPL_OMR2BAH, lah); dcr_write(port->dcrs, DCRO_PEGPL_OMR2BAL, lal); dcr_write(port->dcrs, DCRO_PEGPL_OMR2MSKH, 0x7fffffff); - /* Note that 3 here means enabled | single region */ - dcr_write(port->dcrs, DCRO_PEGPL_OMR2MSKL, sa | 3); + dcr_write(port->dcrs, DCRO_PEGPL_OMR2MSKL, sa); break; case 2: out_le32(mbase + PECFG_POM2LAH, pciah); @@ -1591,8 +1632,7 @@ static int __init ppc4xx_setup_one_pciex_POM(struct ppc4xx_pciex_port *port, dcr_write(port->dcrs, DCRO_PEGPL_OMR3BAH, lah); dcr_write(port->dcrs, DCRO_PEGPL_OMR3BAL, lal); dcr_write(port->dcrs, DCRO_PEGPL_OMR3MSKH, 0x7fffffff); - /* Note that 3 here means enabled | IO space !!! */ - dcr_write(port->dcrs, DCRO_PEGPL_OMR3MSKL, sa | 3); + dcr_write(port->dcrs, DCRO_PEGPL_OMR3MSKL, sa); break; } @@ -1693,6 +1733,9 @@ static void __init ppc4xx_configure_pciex_PIMs(struct ppc4xx_pciex_port *port, if (res->flags & IORESOURCE_PREFETCH) sa |= 0x8; + if (of_device_is_compatible(port->node, "ibm,plb-pciex-460sx")) + sa |= PCI_BASE_ADDRESS_MEM_TYPE_64; + out_le32(mbase + PECFG_BAR0HMPA, RES_TO_U32_HIGH(sa)); out_le32(mbase + PECFG_BAR0LMPA, RES_TO_U32_LOW(sa)); @@ -1854,6 +1897,10 @@ static void __init ppc4xx_pciex_port_setup_hose(struct ppc4xx_pciex_port *port) } out_le16(mbase + 0x202, val); + /* Enable Bus master, memory, and io space */ + if (of_device_is_compatible(port->node, "ibm,plb-pciex-460sx")) + out_le16(mbase + 0x204, 0x7); + if (!port->endpoint) { /* Set Class Code to PCI-PCI bridge and Revision Id to 1 */ out_le32(mbase + 0x208, 0x06040001); diff --git a/arch/powerpc/sysdev/ppc4xx_pci.h b/arch/powerpc/sysdev/ppc4xx_pci.h index 56d9e5d..28f0bb0 100644 --- a/arch/powerpc/sysdev/ppc4xx_pci.h +++ b/arch/powerpc/sysdev/ppc4xx_pci.h @@ -464,6 +464,9 @@ #define PECFG_POM2LAL 0x390 #define PECFG_POM2LAH 0x394 +/* 460sx only */ +#define PECFG_460SX_DLLSTA 0x3f8 + /* SDR Bit Mappings */ #define PESDRx_RCSSET_HLDPLB 0x10000000 #define PESDRx_RCSSET_RSTGU 0x01000000
Adds a register to the config space for the 460sx. Changes the vc0 detect to a pll detect. maps configuration space to test the link status. changes the setup to enable gen2 devices to operate at gen2 speeds. fixes mapping that was not correct for the 460sx. tested on the 460sx eiger and custom board Signed-off-by: Ayman El-Khashab <ayman@elkhashab.com> --- arch/powerpc/sysdev/ppc4xx_pci.c | 83 +++++++++++++++++++++++++++++-------- arch/powerpc/sysdev/ppc4xx_pci.h | 3 + 2 files changed, 68 insertions(+), 18 deletions(-)