Dear Adam Graham,
In message <[EMAIL PROTECTED]> you wrote:
>
> +/*
> + * FPGA read/write helper macros
> + */
> +#define FPGA_READ(offset) ({ \
> + in_8((void *)(CFG_FPGA_BASE + offset)); })
> +#define FPGA_WRITE(offset, data) ({ \
> + out_8((void *)(CFG_FPGA_BASE + offset), data); })
> +
> +/*
> + * CPLD read/write helper macros
> + */
> +#define CPLD_READ(offset) ({ \
> + out_8((void *)(CFG_CPLD_ADDR), offset); \
> + in_8((void *)(CFG_CPLD_DATA)); })
> +#define CPLD_WRITE(offset, data) ({ \
> + out_8((void *)(CFG_CPLD_ADDR), offset); \
> + out_8((void *)(CFG_CPLD_DATA), data); })
Please make these inline functions instead. (See CodingStyle
document: "Generally, inline functions are preferable to macros
resembling functions.").
> + mtdcr(uic0cr, 0x00000005); /* ATI & UIC1 crit are critical */
It would be nice to use symbolic constatnts instead of hardcoded
numeric valies here.
> + /*
> + * Configure PFC (Pin Function Control) registers
> + * UART0: 4 pins
> + */
> + mtsdr(SDR0_PFC1, 0x00040000);
Ditto.
> + /* Enable PCI host functionality in SDR0_PCI0 */
> + mtsdr(SDR0_PCI0, 0xe0000000);
Ditto.
> + return 0;
> +}
Hm... most of this code looks suspiciously similar to what we have in
"board/amcc/canyonlands/canyonlands.c" - which is not a big surprise,
given the fact how closely related these two boards are.
Do we really need a separate board directory for arches?
Wouldn't it make more sense to use a common source tree for both,
like we do in Linux, too?
> +/*
> + * pci_target_init
> + *
> + * The bootstrap configuration provides default settings for the pci
> + * inbound map (PIM). But the bootstrap config choices are limited and
> + * may not be sufficient for a given board.
> + */
> +#if defined(CONFIG_PCI) && defined(CFG_PCI_TARGET_INIT)
> +void pci_target_init(struct pci_controller *hose)
> +{
> + /*
> + * Disable everything
> + */
> + out_le32((void *)PCIX0_PIM0SA, 0); /* disable */
> + out_le32((void *)PCIX0_PIM1SA, 0); /* disable */
> + out_le32((void *)PCIX0_PIM2SA, 0); /* disable */
> + out_le32((void *)PCIX0_EROMBA, 0); /* disable expansion rom */
> +
> + /*
> + * Map all of SDRAM to PCI address 0x0000_0000. Note that the 440
> + * strapping options to not support sizes such as 128/256 MB.
> + */
> + out_le32((void *)PCIX0_PIM0LAL, CFG_SDRAM_BASE);
> + out_le32((void *)PCIX0_PIM0LAH, 0);
> + out_le32((void *)PCIX0_PIM0SA, ~(gd->ram_size - 1) | 1);
> + out_le32((void *)PCIX0_BAR0, 0);
> +
> + /*
> + * Program the board's subsystem id/vendor id
> + */
> + out_le16((void *)PCIX0_SBSYSVID, CFG_PCI_SUBSYS_VENDORID);
> + out_le16((void *)PCIX0_SBSYSID, CFG_PCI_SUBSYS_DEVICEID);
> +
> + out_le16((void *)PCIX0_CMD, in16r(PCIX0_CMD) | PCI_COMMAND_MEMORY);
> +}
> +#endif /* defined(CONFIG_PCI) && defined(CFG_PCI_TARGET_INIT) */
> +
> +#if defined(CONFIG_PCI)
> +/*
> + * is_pci_host
> + *
> + * This routine is called to determine if a pci scan should be
> + * performed. With various hardware environments (especially cPCI and
> + * PPMC) it's insufficient to depend on the state of the arbiter enable
> + * bit in the strap register, or generic host/adapter assumptions.
> + *
> + * Rather than hard-code a bad assumption in the general 440 code, the
> + * 440 pci code requires the board to decide at runtime.
> + *
> + * Return 0 for adapter mode, non-zero for host (monarch) mode.
> + */
> +int is_pci_host(struct pci_controller *hose)
> +{
> + /* Board is always configured as host. */
> + return 1;
> +}
> +
> +static struct pci_controller pcie_hose[2] = { {0}, {0} };
> +
> +void pcie_setup_hoses(int busno)
> +{
> + struct pci_controller *hose;
> + int i, bus;
> + int ret = 0;
> + char *env;
> + unsigned int delay;
> + int end;
> +
> + /*
> + * assume we're called after the PCIX hose is initialized, which takes
> + * bus ID 0 and therefore start numbering PCIe's from 1.
> + */
> + bus = busno;
> +
> +#if defined(CONFIG_RAPIDIO)
> + end = 0;
> +#else
> + end = 1;
> +#endif
> +
> + for (i = 0; i <= end; i++) {
> +
> + if (is_end_point(i))
> + ret = ppc4xx_init_pcie_endport(i);
> + else
> + ret = ppc4xx_init_pcie_rootport(i);
> + if (ret) {
> + printf("PCIE%d: initialization as %s failed\n", i,
> + is_end_point(i) ? "endpoint" : "root-complex");
> + continue;
> + }
> +
> + hose = &pcie_hose[i];
> + hose->first_busno = bus;
> + hose->last_busno = bus;
> + hose->current_busno = bus;
> +
> + /* setup mem resource */
> + pci_set_region(hose->regions + 0,
> + CFG_PCIE_MEMBASE + i * CFG_PCIE_MEMSIZE,
> + CFG_PCIE_MEMBASE + i * CFG_PCIE_MEMSIZE,
> + CFG_PCIE_MEMSIZE,
> + PCI_REGION_MEM);
> + hose->region_count = 1;
> + pci_register_hose(hose);
> +
> + if (is_end_point(i)) {
> + ppc4xx_setup_pcie_endpoint(hose, i);
> + /*
> + * Reson for no scanning is endpoint can not generate
> + * upstream configuration accesses.
> + */
> + } else {
> + ppc4xx_setup_pcie_rootpoint(hose, i);
> + env = getenv("pciscandelay");
> + if (env != NULL) {
> + delay = simple_strtoul(env, NULL, 10);
> + if (delay > 5)
> + printf("Warning, expect noticable "
> + "delay before PCIe scan due "
> + "to 'pciscandelay' value!\n");
> + mdelay(delay * 1000);
> + }
> +
> + /*
> + * Config access can only go down stream
> + */
> + hose->last_busno = pci_hose_scan(hose);
> + bus = hose->last_busno + 1;
> + }
> + }
> +}
> +#endif /* CONFIG_PCI */
All this looks liek verbatim copies of the canyonlands code.
Similar for the rest of the code.
I think we should avoid such an extensive code duplication.
NAK.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: [EMAIL PROTECTED]
"There's always been Tower of Babel sort of bickering inside Unix,
but this is the most extreme form ever. This means at least several
years of confusion." - Bill Gates, founder and chairman of Microsoft,
about the Open Systems Foundation
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
