[U-Boot] [PATCH 2/2] Stamp9261 board support

[Wolfgang Denk]

Dear Michael Roth,

In message <1233757799-1165-3-git-send-email-mroth at nessie.de> you wrote:
> Initial support for Taskit Stamp9261 board - based on an
> AT91SAM9261 SoC from Atmel.
> 
> Currently only a Stamp9261 with 64MB RAM fitted together
> with a Stamp-Adaptor on a Panel-Card EVB is supported.
> 
> Signed-off-by: Michael Roth <mroth at nessie.de>
...

> diff --git a/board/stamp9261/led.c b/board/stamp9261/led.c
> new file mode 100644
> index 0000000..367db20
> --- /dev/null
> +++ b/board/stamp9261/led.c

NAK. Enough is enough, and this is more than enough.

We already have 6 mostly identical copies of board/atmel/at91*/led.c
files. I will not accept adding yet another of these.

Please move the common stuff into common code with just board specific
cunfigurable settings.


> diff --git a/board/stamp9261/lowlevel_init.S b/board/stamp9261/lowlevel_init.S
> new file mode 100644
> index 0000000..0531091
> --- /dev/null
> +++ b/board/stamp9261/lowlevel_init.S
...
> +#define SRAM_BASE		0x300000	/* Internal SRAM C */
> +
> +#define SDRAM_BASE		0x20000000	/* External SDRAM */
> +
> +#define SYS_BASE		0xffffea00	/* System Controller */
> +
> +#define SDRAMC_OFFSET		(0xffffea00 - SYS_BASE)
> +#define SDRAMC_MR_OFFSET	(0xffffea00 - SDRAMC_OFFSET - SYS_BASE)
> +#define SDRAMC_TR_OFFSET	(0xffffea04 - SDRAMC_OFFSET - SYS_BASE)
> +#define SDRAMC_CR_OFFSET	(0xffffea08 - SDRAMC_OFFSET - SYS_BASE)
> +
> +#define SMC0_OFFSET		(0xffffec00 - SYS_BASE)
> +
> +#define MATRIX_OFFSET		(0xffffee00 - SYS_BASE)
> +#define MATRIX_SCFG3_OFFSET	(0xffffee10 - MATRIX_OFFSET - SYS_BASE)
> +#define SLOT_CYCLE(a)		((a)<<0)
> +#define DEFMSTR_TYPE(a)		((a)<<16)
> +#define FIXED_DEFMSTR(a)	((a)<<18)
> +#define EBI_CSA_OFFSET		(0xffffee30 - MATRIX_OFFSET - SYS_BASE)
> +#define EBI_CS1A(a)		((a)<<1)
...

etc.

NAK. lowlevel_init.S is not the place for such #defines. Either these
are common to the  processor,  than  they  should  be  part  of  some
processor  header  file, or they are board specific, then they should
be part of your board config file.


> diff --git a/board/stamp9261/stamp9261.c b/board/stamp9261/stamp9261.c
> new file mode 100644
> index 0000000..51fe785
> --- /dev/null
> +++ b/board/stamp9261/stamp9261.c
...
> +/*
> + * Serial Port initialisation
> + */
> +static void stamp9261_serial_init(void)
> +{
> +#ifdef CONFIG_ATMEL_USART
> +#ifdef CONFIG_USART0
> +	at91_set_A_periph(AT91_PIN_PC8, 1);		/* TXD0 */
> +	at91_set_A_periph(AT91_PIN_PC9, 0);		/* RXD0 */
> +	at91_sys_write(AT91_PMC_PCER, 1 << AT91_ID_US0);
> +#endif
> +#ifdef CONFIG_USART1
> +	at91_set_A_periph(AT91_PIN_PC12, 1);		/* TXD1 */
> +	at91_set_A_periph(AT91_PIN_PC13, 0);		/* RXD1 */
> +	at91_sys_write(AT91_PMC_PCER, 1 << AT91_ID_US1);
> +#endif
> +#ifdef CONFIG_USART2
> +	at91_set_A_periph(AT91_PIN_PC14, 1);		/* TXD2 */
> +	at91_set_A_periph(AT91_PIN_PC15, 0);		/* RXD2 */
> +	at91_sys_write(AT91_PMC_PCER, 1 << AT91_ID_US2);
> +#endif
> +#ifdef CONFIG_USART3	/* DBGU */
> +	at91_set_A_periph(AT91_PIN_PA10, 1);		/* DTXD */
> +	at91_set_A_periph(AT91_PIN_PA9, 0);		/* DRXD */
> +	at91_sys_write(AT91_PMC_PCER, 1 << AT91_ID_SYS);
> +#endif
> +#endif
> +}

Error handling if CONFIG_ATMEL_USART or if none of CONFIG_USART[0-3]
is defined?

> +int board_init(void)
> +{
> +	/* Enable Ctrlc */
> +	console_init_f();
> +
> +	/* arch number of Stamp9261 on Panel-Card EVB */
> +	gd->bd->bi_arch_number = 2056;	/* FIXME: Change to MACH_TYPE_STAMP9261_PC_EVB as soon as include/asm-arm/mach-types.h is updated */

Maximum line length exceeded.

> +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 auto-size the RAM instead of hardcoding it.

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

No. Please do not do this. This is in violation with U-Boot
implementation requirements. The network interface MUST NOT be
initialized if no network related command is used in U-Boot.
See for example http://www.denx.de/wiki/U-Boot/DesignRequirements

> diff --git a/include/configs/stamp9261.h b/include/configs/stamp9261.h
> new file mode 100644
> index 0000000..ab825e2
...
> +/*
> + * Clocks
> + *
> + * Supported values for AT91_CPU_CLOCK are: 161938286, 199916308, 239616000

Maximum line length exceeded.

> +#define CONFIG_CMDLINE_TAG	/* enable command line passing */
> +#define CONFIG_SETUP_MEMORY_TAGS /* tells memory configuration to the kernel */
> +#define CONFIG_INITRD_TAG	/* tells where the initrd can be found */

Maximum line length exceeded.

> +/*
> + * SDRAM
> + */
> +#define CONFIG_NR_DRAM_BANKS	1
> +#define PHYS_SDRAM		0x20000000
> +#define PHYS_SDRAM_SIZE		0x04000000	/* 64 megs */

Please use get_ram_size() instead.


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
The first 90% of a project takes 90% of the time, the last 10%  takes
the other 90% of the time.