[U-Boot] [PATCH] powerpc: add support for the Freescale P1022DS reference board

[Scott Wood <scottwood@freescale.com>]

On 05/26/2010 02:34 PM, Timur Tabi wrote:
> Scott Wood wrote:
>
>> Which is relevant, given that you're whipping out a big scary-looking
>> prototype as a reason to avoid refactoring. :-)
>
> So instead of this:
>
> configure_pci(PCIE1, "PCIE1", "Slot 1", pcie_ep, num, LAW_TRGT_IF_PCIE_1,
> CONFIG_SYS_PCIE1_MEM_PHYS, LAW_SIZE_512M, CONFIG_SYS_PCIE1_IO_PHYS,
> LAW_SIZE_64K,&pci_info[num],&pcie1_hose);
>
> You want this instead:
>
> struct {
> 	enum srds_prtcl pci;
> 	const char *name;
> 	const char *target;
> 	int endpoint;
> 	int first_free_busno;
> 	enum law_trgt_if law;
> 	phys_addr_t mem_addr;
> 	enum law_size mem_size;
> 	phys_addr_t io_addr;
> 	enum law_size io_size;
> 	struct fsl_pci_info *pci_info;
> 	struct pci_controller *hose;
> } x;
>
> x.pci = PCIE1;
> x.name = "PCIE1";
> x.target = "Slot 1";
> x.endpoint =- pcie_ep;
> x.first_free_busno = num;
> x.law = LAW_TRGT_IF_PCIE_1;
> x.mem_addr = CONFIG_SYS_PCIE1_MEM_PHYS;
> x.mem_size = LAW_SIZE_512M;
> x.io_addr = CONFIG_SYS_PCIE1_IO_PHYS;
> x.io_size = LAW_SIZE_64K;
> x.pci_info =&pci_info[num];
> x.hose =&pcie1_hose;
>
> configure_pci(&x);
>
> I don't see how this is an improvement.

Much of that struct could be defined statically, with the board or soc 
file providing an array of PCI info structs.  Look at how the 83xx PCI 
code does it -- you'll need more than just the generic u-boot pci_region 
struct, though you could have it as a member of the hw-specific struct.

A few of those things don't belong there -- I think first_free_busno 
should be a static variable inside the pci setup function, for example 
(does Linux still even need separate bus number spaces on different 
hoses?).  pcie_ep should just be a local variable.  The hose could maybe 
just be an uninitialized member of the struct instead of a pointer to an 
arbitrary other symbol.

-Scott