[U-Boot] [PATCH 1/3] ppc4xx: Initial support of the AMCC dual PPC460GT Arches reference board.

[Wolfgang Denk]

Dear Adam Graham,

In message <1222243872-15226-1-git-send-email-agraham at 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);
> +		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: wd at denx.de
"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