[U-Boot] [PATCH 2/2 v3] powerpc/c29xpcie: add support for C29XPCIE board

[Scott Wood]

On 07/02/2013 01:39:09 AM, Po Liu wrote:
> +int board_early_init_r(void)
> +{
> +	const unsigned long flashbase = CONFIG_SYS_FLASH_BASE;
> +	const u8 flash_esel = find_tlb_idx((void *)flashbase, 1);
> +
> +	/*
> +	 * Remap Boot flash region to caching-inhibited
> +	 * so that flash can be erased properly.
> +	 */
> +
> +	/* Flush d-cache and invalidate i-cache of any FLASH data */
> +	flush_dcache();
> +	invalidate_icache();
> +
> +	/* invalidate existing TLB entry for flash */
> +	disable_tlb(flash_esel);
> +
> +	set_tlb(1, flashbase, CONFIG_SYS_FLASH_BASE_PHYS,
> +			MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
> +			0, flash_esel, BOOKE_PAGESZ_64M, 1);

Don't set MAS3_SX on I/O regions.

> +#if defined(CONFIG_OF_BOARD_SETUP)
> +void fdt_del_sec(void *blob, int offset)
> +{
> +	int nodeoff = 0;
> +
> +	while ((nodeoff = fdt_node_offset_by_compat_reg(blob,  
> "fsl,sec-v6.0",
> +			CONFIG_SYS_CCSRBAR_PHYS +  
> CONFIG_SYS_FSL_SEC_OFFSET
> +			+ offset * 0x20000)) >= 0) {
> +			fdt_del_node(blob, nodeoff);
> +			offset++;
> +		}
> +}

Whitespace

> +static u8 __cpld_read(unsigned int reg)
> +{
> +	void *p = (void *)CONFIG_SYS_CPLD_BASE;
> +
> +	return in_8(p + reg);
> +}
> +
> +u8 cpld_read(unsigned int reg) __attribute__((weak,  
> alias("__cpld_read")));
> +
> +static void __cpld_write(unsigned int reg, u8 value)
> +{
> +	void *p = (void *)CONFIG_SYS_CPLD_BASE;
> +
> +	out_8(p + reg, value);
> +}
> +
> +void cpld_write(unsigned int reg, u8 value)
> +	__attribute__((weak, alias("__cpld_write")));

Why does this need to be weak?  Where do you expect to call  
cpld_read/write that isn't specific to this board?  What sort of  
behavior could a non-board-specific caller possibly expect?

> +/**
> + * Set the boot bank to the alternate bank
> + */
> +void __cpld_set_altbank(u8 banksel)
> +{
> +	u8 reg11 = CPLD_READ(flhcsr);
> +
> +	switch (banksel) {
> +	case 1:
> +		CPLD_WRITE(flhcsr, (reg11 & CPLD_BANKSEL_MASK)
> +			| CPLD_BANKSEL_EN | CPLD_SELECT_BANK1);
> +		break;
> +	case 2:
> +		CPLD_WRITE(flhcsr, (reg11 & CPLD_BANKSEL_MASK)
> +			| CPLD_BANKSEL_EN | CPLD_SELECT_BANK2);
> +		break;
> +	case 3:
> +		CPLD_WRITE(flhcsr, (reg11 & CPLD_BANKSEL_MASK)
> +			| CPLD_BANKSEL_EN | CPLD_SELECT_BANK3);
> +		break;
> +	case 4:
> +		CPLD_WRITE(flhcsr, (reg11 & CPLD_BANKSEL_MASK)
> +			| CPLD_BANKSEL_EN | CPLD_SELECT_BANK4);
> +		break;

So there's a __cpld_write, a cpld_write, *and* a CPLD_WRITE?

> +#define CPLD_READ(reg) cpld_read(offsetof(struct cpld_data, reg))
> +#define CPLD_WRITE(reg, value) cpld_write(offsetof(struct cpld_data,  
> reg), \
> +						value)

Ugh.  Why not just initialize a pointer to CONFIG_SYS_CPLD_BASE, and use
normal I/O accessors on that, just like every other driver that isn't  
the
p2041rdb CPLD code you copied this from?

> +struct law_entry law_table[] = {
> +	SET_LAW(CONFIG_SYS_FLASH_BASE_PHYS, LAW_SIZE_64M,  
> LAW_TRGT_IF_IFC),
> +	SET_LAW(CONFIG_SYS_CPLD_BASE_PHYS, LAW_SIZE_4K,  
> LAW_TRGT_IF_IFC),
> +	SET_LAW(CONFIG_SYS_NAND_BASE_PHYS, LAW_SIZE_1M,  
> LAW_TRGT_IF_IFC),

The NAND SRAM window is 16K.  There is no reason to make the LAW 1M.

> +	/* TLB 1 */
> +	/* *I*** - Covers boot page */
> +	SET_TLB_ENTRY(1, 0xfffff000, 0xfffff000,
> +			MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
> +			0, 0, BOOKE_PAGESZ_4K, 1),

Comment says *I*** but code says *I*G*.

Do we still use this mapping at all?

> +	/* *I*G* - CCSRBAR */
> +	SET_TLB_ENTRY(1, CONFIG_SYS_CCSRBAR, CONFIG_SYS_CCSRBAR_PHYS,
> +			MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
> +			0, 1, BOOKE_PAGESZ_1M, 1),
> +
> +	SET_TLB_ENTRY(1, CONFIG_SYS_FLASH_BASE,  
> CONFIG_SYS_FLASH_BASE_PHYS,
> +			MAS3_SX|MAS3_SR, MAS2_W|MAS2_G,
> +			0, 2, BOOKE_PAGESZ_64M, 1),

Why do most of these entries have a comment but some (like this flash
entry) don't?  Be consistent.  Probably better to remove all of these
comments since they don't say anything that isn't clear from the code,
and are often wrong.

> +	SET_TLB_ENTRY(1, CONFIG_SYS_NAND_BASE,  
> CONFIG_SYS_NAND_BASE_PHYS,
> +			MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
> +			0, 6, BOOKE_PAGESZ_1M, 1),

Again, this can be 16K.

> +	/* *I*G - platform SRAM */
> +	SET_TLB_ENTRY(1, CONFIG_SYS_PLATFORM_SRAM_BASE,
> +			CONFIG_SYS_PLATFORM_SRAM_BASE_PHYS,
> +			MAS3_SX|MAS3_SW|MAS3_SR, 0,
> +			0, 7, BOOKE_PAGESZ_256K, 1),
> +	SET_TLB_ENTRY(1, CONFIG_SYS_PLATFORM_SRAM_BASE + 0x40000,
> +			CONFIG_SYS_PLATFORM_SRAM_BASE_PHYS + 0x40000,
> +			MAS3_SX|MAS3_SW|MAS3_SR, 0,
> +			0, 8, BOOKE_PAGESZ_256K, 1),

Comment says *I*G but it isn't.

> +/* Use the HUSH parser */
> +#define CONFIG_SYS_HUSH_PARSER
> +#ifdef	CONFIG_SYS_HUSH_PARSER
> +#define CONFIG_SYS_PROMPT_HUSH_PS2 "> "
> +#endif

Again, "Please go through this file and only retain things which  
actually continue to make
sense."

This is a pointless ifdef, and a pointless define of
CONFIG_SYS_PROMPT_HUSH_PS2 since that's the same as the default.

> +/*
> + * Internal Definitions
> + *
> + * Boot Flags
> + */
> +#define BOOTFLAG_COLD	0x01		/* Normal Power-On:  
> Boot from FLASH */
> +#define BOOTFLAG_WARM	0x02		/* Software reboot */

Sigh.

-Scott