Dear Adam Graham,
In message 1222243872-15226-1-git-send-email-agraham@amcc.com 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);elseret = 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