lkml.org 
[lkml]   [2011]   [Jul]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 1/1] powerpc/4xx: enable and fix pcie gen1/gen2 on the 460sx
    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


    \
     
     \ /
      Last update: 2011-07-14 03:19    [W:0.034 / U:29.420 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site