Re: [PATCH 1/1] powerpc/4xx: enable and fix pcie gen1/gen2 on the 460sx

[Ayman El-Khashab <ayman@elkhashab.com>]

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