Subject: Re: [PATCH] powerpc: Add MPC837x PCIE controller RC mode support

From: Arnd Bergmann <[email-masked]>
Date: Wed, 2 Jan 2008 12:53:07 +0100

On Wednesday 02 January 2008, Li Li wrote:
>  #ifdef CONFIG_PCI
> -       for_each_compatible_node(np, "pci", "fsl,mpc8349-pci")
> -               mpc83xx_add_bridge(np);
> +       for_each_compatible_node(np, "pci", "fsl,mpc8349-pci") {
> +               if (primary_pci_bus) {
> +                       mpc83xx_add_bridge(np, PPC_83XX_PCI | PPC_83XX_PCI_PRIMARY);
> +                       primary_pci_bus = 0;
> +               } else
> +                       mpc83xx_add_bridge(np, PPC_83XX_PCI);
> +       }
> +
> +       for_each_compatible_node(np, "pci", "fsl,mpc8377-pcie") {
> +               if (primary_pci_bus) {
> +                       mpc83xx_add_bridge(np, PPC_83XX_PCIE | PPC_83XX_PCI_PRIMARY);
> +                       primary_pci_bus = 0;
> +               } else
> +                       mpc83xx_add_bridge(np, PPC_83XX_PCIE);
> +       }
> +
> +       ppc_md.pci_exclude_device = mpc837x_exclude_device;
>  #endif

A few comments on how I think this can be improved:

* Instead of duplicating this code across all platforms, make a single
function that does the probing in one place (including the #ifdef
CONFIG_PCI).

* Better yet, get rid of mpc83xx_add_bridge and make it possible for the
mpc83xx code to use the of_pci_phb_driver from
arch/powerpc/kernel/of_platform.c. This used to be impossible because
of the differences  between 32 and 64 bit code for PCI probing, but
I think with the work than benh put into unifying the two, we should
be pretty close now.

* The detection method for the primary bus is somewhat fragile, because
we depend on the order of the nodes in the device tree, which is not
specified to have a meaning. /Maybe/ there could be a property in
(at most) one of the PCI host bridge nodes saying that this specific bus
is the primary one.

* Since you are using exactly the same probing code for pci and pcie,
you may want to check for a more generic "compatible" property than
the specific model, so you can use the same code for both.

	Arnd <><