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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
