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