[U-Boot] [RFC] [PATCH] at91: add support for PM9263 board

[Wolfgang Denk]

Dear Jean-Christophe PLAGNIOL-VILLARD,

In message <1233785700-31051-1-git-send-email-plagnioj at jcrosoft.com> you wrote:
> From: Ilko Iliev <iliev at ronetix.at>
> 
> This patch adds support for the PM9263 board of Ronetix GmbH (www.ronetix.at)
> With the necessary to boot the board from NOR
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I cannot parse this. What do you want to express?

> index ec6ad5d..122e23a 100644
> --- a/cpu/arm926ejs/at91/lowlevel_init.S
> +++ b/board/ronetix/pm9263/led.c

NAK.

I will not accept yet another copy of this file. Please create common
code that can be configured as needed.

> diff --git a/board/ronetix/pm9263/partition.c b/board/ronetix/pm9263/partition.c
> new file mode 100644
> index 0000000..95ac398
> --- /dev/null
> +++ b/board/ronetix/pm9263/partition.c

NAK.

I will not accept yet another copy of this file. We have already 7 of
these, most of them identical, and the remaining differences minimal.
Please create common code that can be configured as needed.


> diff --git a/board/ronetix/pm9263/pm9263.c b/board/ronetix/pm9263/pm9263.c
> new file mode 100644
> index 0000000..7bda1b8
...

> +/* ------------------------------------------------------------------------- */
> +/*
> + * Miscelaneous platform dependent initialisations
> + */
> +
> +static void pm9263_serial_hw_init(void)
> +{
> +#ifdef CONFIG_USART0
> +	at91_set_A_periph(AT91_PIN_PA26, 1);		/* TXD0 */
> +	at91_set_A_periph(AT91_PIN_PA27, 0);		/* RXD0 */
> +	at91_sys_write(AT91_PMC_PCER, 1 << AT91_ID_US0);
> +#endif
> +
> +#ifdef CONFIG_USART1
> +	at91_set_A_periph(AT91_PIN_PD0, 1);		/* TXD1 */
> +	at91_set_A_periph(AT91_PIN_PD1, 0);		/* RXD1 */
> +	at91_sys_write(AT91_PMC_PCER, 1 << AT91_ID_US1);
> +#endif
> +
> +#ifdef CONFIG_USART2
> +	at91_set_A_periph(AT91_PIN_PD2, 1);		/* TXD2 */
> +	at91_set_A_periph(AT91_PIN_PD3, 0);		/* RXD2 */
> +	at91_sys_write(AT91_PMC_PCER, 1 << AT91_ID_US2);
> +#endif
> +
> +#ifdef CONFIG_USART3	/* DBGU */
> +	at91_set_A_periph(AT91_PIN_PC30, 0);		/* DRXD */
> +	at91_set_A_periph(AT91_PIN_PC31, 1);		/* DTXD */
> +	at91_sys_write(AT91_PMC_PCER, 1 << AT91_ID_SYS);
> +#endif
> +}

I think I've seent his code a couple of times before, too. Last time
in another patch earlier today. Seems there are at least 8 files in
U-Boot that contain such a sequence, with no or just minimal
differences.

Should we not clean this up, too?


> +static int pm9263_lcd_hw_psram_init(void)
> +{
...
> +	if ((readw(PHYS_PSRAM) != 0x1234) || (readw(PHYS_PSRAM+2) != 0x5678))
> +	{

Incorrect brace style.

> +		writew(0x5678, PHYS_PSRAM+2);
> +		if ((readw(PHYS_PSRAM) != 0x1234)
> +		   || (readw(PHYS_PSRAM + 2) != 0x5678))
> +			return 1;

I recommend to use curly braces because of the multi-line "if".

Also, please put the "||" at the end of line 1.

> +int dram_init(void)
> +{
> +	gd->bd->bi_dram[0].start = PHYS_SDRAM;
> +	gd->bd->bi_dram[0].size = PHYS_SDRAM_SIZE;
> +	return 0;
> +}

You should not hard-wire the memory size. You should auto-size it.

> +#ifdef CONFIG_RESET_PHY_R
> +void reset_phy(void)
> +{
> +#ifdef CONFIG_MACB
> +	/*
> +	 * Initialize ethernet HW addr prior to starting Linux,
> +	 * needed for nfsroot
> +	 */
> +	eth_init(gd->bd);
> +#endif

Be careful. You mustnot initialize the ehternet port unless a U-Boot
network command is being used. And you must shut it down after that.

> +#ifdef CONFIG_DISPLAY_BOARDINFO
> +int checkboard (void)
> +{
> +	char *ss;
> +
> +	uint32_t reg, diva, mula;
> +	uint32_t val, mdiv, pres, crystal;
> +	char buf[32];
> +
> +	reg = at91_sys_read(AT91_CKGR_PLLAR);
> +	mula = ((reg >> 16) & 0x7FF) + 1;
> +	diva = reg & 0xFF;
> +
> +	reg = at91_sys_read(AT91_PMC_MCKR);
> +	mdiv = (reg >> 8) & 3;
> +	pres = (reg >> 2) & 7;

Maybe a comment would help to explain what this code is doing?

> +	printf("Video memory : 0x%08lX %s\n", gd->fb_base, ss );

No space before the paren.

> +	printf ("\n");

Please use consistent style: either

	printf(...);
or
	printf (...);

bot on;t mix these (this applies to all function calls).

> diff --git a/cpu/arm926ejs/at91/lowlevel_init.S b/board/ronetix/pm9263/u-boot.lds
> similarity index 54%
> copy from cpu/arm926ejs/at91/lowlevel_init.S
> copy to board/ronetix/pm9263/u-boot.lds
> index ec6ad5d..443f180 100644
> --- a/cpu/arm926ejs/at91/lowlevel_init.S
> +++ b/board/ronetix/pm9263/u-boot.lds

That's a pretty funny diff, isn't it? lowlevel_init.S versus
u-boot.lds ...

> diff --git a/include/configs/pm9263.h b/include/configs/pm9263.h
> new file mode 100644
> index 0000000..4a20bff
> --- /dev/null
> +++ b/include/configs/pm9263.h
...
> +#define AT91_MAIN_CLOCK		(PM9263_CRYSTAL / MASTER_PLL_DIV * MASTER_PLL_MUL)

Maximum line lenght exceeded. Check all files, there are more places
like this one!

> +#define AT91_MASTER_CLOCK	(AT91_MAIN_CLOCK / MAIN_PLL_DIV)
> +#define CONFIG_SYS_AT91_PLLB	0x10483E0E	/* PLLB settings for USB */
> +#define CONFIG_SYS_HZ		1000000

NAK.

CONFIG_SYS_HZ must be 1000.

> +#define CFG_MONITOR_BASE	(CFG_DATAFLASH_LOGIC_ADDR_CS0 + 0x8400)
> +#define CONFIG_ENV_OFFSET	0x4200
> +#define CONFIG_ENV_ADDR		(CFG_DATAFLASH_LOGIC_ADDR_CS0 + CONFIG_ENV_OFFSET)
> +#define CONFIG_ENV_SIZE		0x4200
> +#define CONFIG_BOOTCOMMAND	"cp.b 0xC0042000 0x22000000 0x210000; bootm"

No need for 0x in U-Boot commands. This is just wasting environment
memory.

> +#define CONFIG_BOOTCOMMAND	"nand read 0x22000000 0xA0000 0x200000; bootm"

Ditto.

> +#define ROUND(A, B)		(((A) + (B)) & ~((B) - 1))

Do we really need this redefined in each and every
include/configs/at91*.h file?

> + * Size of malloc() pool
> + */
> +#define CONFIG_SYS_MALLOC_LEN	ROUND(3 * CONFIG_ENV_SIZE + 128*1024, 0x1000)

Same here?

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
A supercomputer is a machine that runs an endless loop in 2 seconds.