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

Scott Wood
2010-05-26T19:46:28Z

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