Re: [PATCH 11/25] powerpc: 4xx PLB to PCI Express support

[Stefan Roese <sr@denx.de>]

On Thursday 06 December 2007, Benjamin Herrenschmidt wrote:
> This adds to the previous 2 patches the support for the 4xx PCI Express
> cells as found in the 440SPe revA, revB and 405EX.
>
> Unfortunately, due to significant differences between these, and other
> interesting "features" of those pieces of HW, the code isn't as simple
> as it is for PCI and PCI-X and some of the functions differ significantly
> between the 3 implementations. Thus, not only this code can only support
> those 3 implementations for now and will refuse to operate on any other,
> but there are added ifdef's to avoid the bloat of building a fairly large
> amount of code on platforms that don't need it.
>
> Also, this code currently only supports fully initializing root complex
> nodes, not endpoint. Some more code will have to be lifted from the
> arch/ppc implementation to add the endpoint support, though it's mostly
> differences in memory mapping, and the question on how to represent
> endpoint mode PCI in the device-tree is thus open.
>
> Many thanks to Stefan Roese for testing & fixing up the 405EX bits !

You're welcome. Thank you for doing the "dirty work".

Please find some comments below.

<snip>

> +/* Check that the core has been initied and if not, do it */
> +static int __init ppc4xx_pciex_check_core_init(struct device_node *np)
> +{
> +	static int core_init;
> +	int count = -ENODEV;
> +
> +	if (core_init++)
> +		return 0;
> +
> +#ifdef CONFIG_44x
> +	if (of_device_is_compatible(np, "ibm,plb-pciex-440speA"))
> +		ppc4xx_pciex_hwops = &ppc440speA_pcie_hwops;
> +	else if (of_device_is_compatible(np, "ibm,plb-pciex-440speB"))
> +		ppc4xx_pciex_hwops = &ppc440speB_pcie_hwops;

We need some runtime detection of the 440SPe revision here. There are boards 
out there (e.g. AMCC Yucca) which can be equipped with both PPC 
revisions. :-(

> +#endif /* CONFIG_44x    */
> +#ifdef CONFIG_40x
> +	if (of_device_is_compatible(np, "ibm,plb-pciex-405ex"))
> +		ppc4xx_pciex_hwops = &ppc405ex_pcie_hwops;
> +#endif

Why those #ifdef's? Just code-size reasons, since 40x and 44x will most likely 
never be built into one image? 

Thanks.

Best regards,
Stefan