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

[Stefan Roese]

On Wednesday 24 September 2008, Wolfgang Denk 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.").

ACK.

> > +	mtdcr(uic0cr, 0x00000005);	/* ATI & UIC1 crit are critical */
>
> It would be nice to use symbolic constatnts instead of hardcoded
> numeric valies here.

I don't think that this makes sense here. We would need to add 32 defines 
times the number of UIC's available to make it possible to configure this 
with symbolic constants. That's 128 in the 460EX/GT case. So I vote for 
leaving it as is here.

> > +	/*
> > +	 * Configure PFC (Pin Function Control) registers
> > +	 * UART0: 4 pins
> > +	 */
> > +	mtsdr(SDR0_PFC1, 0x00040000);
>
> Ditto.

ACK. Adam just copied the hardcoded value from Canyonlands, but it is a good 
idea to use a define for this PFCx configuration.

> > +	/* Enable PCI host functionality in SDR0_PCI0 */
> > +	mtsdr(SDR0_PCI0, 0xe0000000);
>
> Ditto.

ACK.

> > +	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?

I'm not so sure here. In Linux it is different. All the 4xx platforms files 
were 100% identical. So it really made no sense to have multiple incarnations 
there. In U-Boot we have some differences. I'm not sure if it makes sense to 
try to move the common code into a separate place and reuse it there. We 
would have to introduce even more #ifdef's in the resulting code to make it 
work on Canyonlands, Glacier & Arches.

OK, functions that are 100% identical between multiple boards are probably 
better moved into such a common directory (perhaps board/amcc/common). The 
PCI(e) related functions come to my mind first.

> > +/*
> > + *  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.

No, not 100% I'm afraid. There are subtile differences here with respect to 
RAPIDIO and the Canyonlands/Glacier distinction in the Canyonlands version.

> Similar for the rest of the code.
>
> I think we should avoid such an extensive code duplication.

Sure, it is always good to not duplicate code. But if the code is not 100% 
identical then I'm unsure what's better. Using more common board code has the 
following advantages and disadvantages:

+ Less code duplication
+ Easier to maintain
- Resulting code is "uglier" by using more #ifdef's
- Makes it harder for custom board ports, since the code needed is
  scattered in more directories

Not sure where to go here. Wolfgang, your call.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================