Thanks Tony, some comments below.
On Thu, Jul 14, 2011 at 11:16:27AM +1000, Tony Breeds wrote:
quoted
+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.
quoted
+ 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.
quoted
- 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.
quoted
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