Re: [PATCH v2 -- fixed changelog] [POWERPC] Add support for Rev. B of PowerPC 440SPe

[Eugene Surovegin <ebs@ebshome.net>]

On Mon, Nov 13, 2006 at 02:31:51PM -0800, Roland Dreier wrote:
> This is mostly updating the PCI Express code to work with the new core
> in the Rev. B chip, which unfortunately has different undocumented
> restrictions on the PLB addresses that can be used from the Rev. A core.
> 
> Also, when adding the cputable entry for 440SPe Rev. B, we need to
> adjust the entry for 440SP Rev. A so that it looks at more bits of the
> PVR.  The 440SPe Rev. B has PVR 53421891, which would have matched the
> old 440SP pattern of 53xxx891.
> 
> This is a cleaned up version of the original work done by Rafal
> Jaworowski <raj@semihalf.com>, who actually suffered through figuring
> out how to avoid the Rev. B chip locking up when using PCIe.
> 
> Signed-off-by: Roland Dreier <rolandd@cisco.com>

[snip]

> +static void ppc440spe_setup_utl(int port)
> +{
> +	void __iomem *utl_base;
> +
> +	/*
> +	 * Map UTL registers at 0xc_1000_0n00
> +	 */
> +	switch (port) {
> +	case 0:
> +		mtdcr(DCRN_PEGPL_REGBAH(PCIE0), 0x0000000c);
> +		mtdcr(DCRN_PEGPL_REGBAL(PCIE0), 0x10000000);
> +		mtdcr(DCRN_PEGPL_REGMSK(PCIE0), 0x00007001);
> +		mtdcr(DCRN_PEGPL_SPECIAL(PCIE0), 0x68782800);
> +		break;
> +
> +	case 1:
> +		mtdcr(DCRN_PEGPL_REGBAH(PCIE1), 0x0000000c);
> +		mtdcr(DCRN_PEGPL_REGBAL(PCIE1), 0x10001000);
> +		mtdcr(DCRN_PEGPL_REGMSK(PCIE1), 0x00007001);
> +		mtdcr(DCRN_PEGPL_SPECIAL(PCIE1), 0x68782800);
> +		break;
> +
> +	case 2:
> +		mtdcr(DCRN_PEGPL_REGBAH(PCIE2), 0x0000000c);
> +		mtdcr(DCRN_PEGPL_REGBAL(PCIE2), 0x10002000);
> +		mtdcr(DCRN_PEGPL_REGMSK(PCIE2), 0x00007001);
> +		mtdcr(DCRN_PEGPL_SPECIAL(PCIE2), 0x68782800);
> +	}

Can this be collapsed into 4 writes instead of 12? DCR accessors 
have supported run-time specified parameters for quite some time.

[snip]

> +	case 0: val = SDR_READ(PESDR0_LOOP); break;
> +	case 1: val = SDR_READ(PESDR1_LOOP); break;
> +	case 2: val = SDR_READ(PESDR2_LOOP); break;

I'm curious, why do we need a switch here? 
SDR_READ() also supports run-time parameters.


[snip]

>  	switch (port) {
>  	case 0:
> -		mtdcr(DCRN_PEGPL_CFGBAH(PCIE0), 0x0000000c);
> -		mtdcr(DCRN_PEGPL_CFGBAL(PCIE0), 0x40000000);
> +		if (core_rev == REV_A) {
> +			mtdcr(DCRN_PEGPL_CFGBAH(PCIE0), 0x0000000c);
> +			mtdcr(DCRN_PEGPL_CFGBAL(PCIE0), 0x40000000);
> +		} else {
> +			mtdcr(DCRN_PEGPL_CFGBAH(PCIE0), 0x0000000d);
> +			mtdcr(DCRN_PEGPL_CFGBAL(PCIE0), 0x00000000);
> +		}
>  		mtdcr(DCRN_PEGPL_CFGMSK(PCIE0), 0xe0000001); /* 512MB region, valid */
>  		break;
>  
>  	case 1:
> -		mtdcr(DCRN_PEGPL_CFGBAH(PCIE1), 0x0000000c);
> -		mtdcr(DCRN_PEGPL_CFGBAL(PCIE1), 0x80000000);
> +		if (core_rev == REV_A) {
> +			mtdcr(DCRN_PEGPL_CFGBAH(PCIE1), 0x0000000c);
> +			mtdcr(DCRN_PEGPL_CFGBAL(PCIE1), 0x80000000);
> +		} else {
> +			mtdcr(DCRN_PEGPL_CFGBAH(PCIE1), 0x0000000d);
> +			mtdcr(DCRN_PEGPL_CFGBAL(PCIE1), 0x20000000);
> +		}
>  		mtdcr(DCRN_PEGPL_CFGMSK(PCIE1), 0xe0000001); /* 512MB region, valid */
>  		break;
>  
>  	case 2:
> -		mtdcr(DCRN_PEGPL_CFGBAH(PCIE2), 0x0000000c);
> -		mtdcr(DCRN_PEGPL_CFGBAL(PCIE2), 0xc0000000);
> +		if (core_rev == REV_A) {
> +			mtdcr(DCRN_PEGPL_CFGBAH(PCIE2), 0x0000000c);
> +			mtdcr(DCRN_PEGPL_CFGBAL(PCIE2), 0xc0000000);
> +		} else {
> +			mtdcr(DCRN_PEGPL_CFGBAH(PCIE2), 0x0000000d);
> +			mtdcr(DCRN_PEGPL_CFGBAL(PCIE2), 0x40000000);
> +		}
>  		mtdcr(DCRN_PEGPL_CFGMSK(PCIE2), 0xe0000001); /* 512MB region, valid */
>  		break;
>  	}


Can we get rid of this switch as well?

There are other places like this in the patch but I think you got my 
idea :).

-- 
Eugene