[U-Boot] [PATCH] arm: at91: at91sam9n12ek: add nandflash/spiflash/mmc/lcd support

Mon Mar 18 07:58:21 CET 2013

Dear Josh Wu,

In message <1363342624-2939-1-git-send-email-josh.wu at atmel.com> you wrote:
> This patch adds at91sam9n12ek support, it enables:
> - dbgu
> - nand with pmecc
> - spi flash
> - mmc
> - lcd

It appears you are adding support for a new board - in this case the
Subject: is misleading, plrase fix.

The missing entry in MAINTAINERS has already been pointed out. Please
also make sure to run your patch through checkpatch - it complains for
example "WARNING: line over 80 characters".  Please fix these, too.

Please also make sure to keep all lists sorted. Bo shen already
mentioned this for the Makefile, but this also applies to lists as
here:

	 #elif defined(CONFIG_AT91SAM9G45) || defined(CONFIG_AT91SAM9M10G45) \
	-		|| defined(CONFIG_AT91SAM9X5)
	+		|| defined(CONFIG_AT91SAM9X5) || defined(CONFIG_AT91SAM9N12)

> +++ b/arch/arm/include/asm/arch-at91/at91sam9n12.h
> @@ -0,0 +1,126 @@
...
> +/*
> + * Peripheral identifiers/interrupts.
> + */
> +#define ATMEL_ID_FIQ	0	/* Advanced Interrupt Controller (FIQ) */
> +#define ATMEL_ID_SYS	1	/* System Controller Interrupt */
> +#define ATMEL_ID_PIOAB	2	/* Parallel I/O Controller A and B */
> +#define ATMEL_ID_PIOCD	3	/* Parallel I/O Controller C and D */
> +#define ATMEL_ID_FUSE	4	/* FUSE Controller */
> +#define ATMEL_ID_USART0	5	/* USART 0 */
> +#define ATMEL_ID_USART1	6	/* USART 1 */
> +#define ATMEL_ID_USART2	7	/* USART 2 */
> +#define ATMEL_ID_USART3	8	/* USART 3 */
> +#define ATMEL_ID_TWI0	9	/* Two-Wire Interface 0 */
> +#define ATMEL_ID_TWI1	10	/* Two-Wire Interface 1 */
> +#define ATMEL_ID_HSMCI	12	/* High Speed Multimedia Card Interface */
> +#define ATMEL_ID_SPI0	13	/* Serial Peripheral Interface 0 */
> +#define ATMEL_ID_SPI1	14	/* Serial Peripheral Interface 1 */
> +#define ATMEL_ID_UART0	15	/* UART 0 */
> +#define ATMEL_ID_UART1	16	/* UART 1 */
> +#define ATMEL_ID_TC01	17	/* Timer Counter 0, 1, 2, 3, 4 and 5 */
> +#define ATMEL_ID_PWM	18	/* Pulse Width Modulation Controller */
> +#define ATMEL_ID_ADC	19	/* ADC Controller */
> +#define ATMEL_ID_DMAC	20	/* DMA Controller */
> +#define ATMEL_ID_UHP	22	/* USB Host */
> +#define ATMEL_ID_UDP	23	/* USB Device */
> +#define ATMEL_ID_LCDC	25	/* LCD Controller */
> +#define ATMEL_ID_SSC	28	/* Synchronous Serial Controller */
> +#define ATMEL_ID_TRNG	30	/* True Random Number Generator */
> +#define ATMEL_ID_IRQ	31	/* Advanced Interrupt Controller */

We already have the very same data in
"arch/arm/include/asm/arch-at91/at91sam9x5.h".

> +/*
> + * User Peripherals physical base addresses.
> + */
> +#define ATMEL_BASE_SPI0		0xf0000000
> +#define ATMEL_BASE_SPI1		0xf0004000
> +#define ATMEL_BASE_HSMCI	0xf0008000
> +#define ATMEL_BASE_SSC		0xf0010000
> +#define ATMEL_BASE_TC012	0xf8008000
> +#define ATMEL_BASE_TC345	0xf800c000
> +#define ATMEL_BASE_TWI0		0xf8010000
> +#define ATMEL_BASE_TWI1		0xf8014000
> +#define ATMEL_BASE_USART0	0xf801c000
> +#define ATMEL_BASE_USART1	0xf8020000
> +#define ATMEL_BASE_USART2	0xf8024000
> +#define ATMEL_BASE_USART3	0xf8028000
> +#define ATMEL_BASE_PWM		0xf8034000
> +#define ATMEL_BASE_LCDC		0xf8038000
> +#define ATMEL_BASE_UDP		0xf803c000
> +#define ATMEL_BASE_UART0	0xf8040000
> +#define ATMEL_BASE_UART1	0xf8044000
> +#define ATMEL_BASE_TRNG		0xf8048000
> +#define ATMEL_BASE_ADC		0xf804c000

Same here.

> +/*
> + * System Peripherals physical base addresses.
> + */
> +#define ATMEL_BASE_FUSE		0xffffdc00
> +#define ATMEL_BASE_MATRIX	0xffffde00
> +#define ATMEL_BASE_PMECC	0xffffe000
> +#define ATMEL_BASE_PMERRLOC	0xffffe600
> +#define ATMEL_BASE_DDRSDRC	0xffffe800
> +#define ATMEL_BASE_SMC		0xffffea00
> +#define ATMEL_BASE_DMAC		0xffffec00
> +#define ATMEL_BASE_AIC		0xfffff000
> +#define ATMEL_BASE_DBGU		0xfffff200
> +#define ATMEL_BASE_PIOA		0xfffff400
> +#define ATMEL_BASE_PIOB		0xfffff600
> +#define ATMEL_BASE_PIOC		0xfffff800
> +#define ATMEL_BASE_PIOD		0xfffffa00
> +#define ATMEL_BASE_PMC		0xfffffc00
> +#define ATMEL_BASE_RSTC		0xfffffe00
> +#define ATMEL_BASE_SHDC		0xfffffe10
> +#define ATMEL_BASE_PIT		0xfffffe30
> +#define ATMEL_BASE_WDT		0xfffffe40
> +#define ATMEL_BASE_SCKCR	0xfffffe50
> +#define ATMEL_BASE_BSCR		0xfffffe54
> +#define ATMEL_BASE_GPBR		0xfffffe60
> +#define ATMEL_BASE_RTC		0xfffffeb0

And here (except for the newly added ATMEL_BASE_FUSE).

Oh, these tables are repeated in
arch/arm/include/asm/arch-at91/at91sam9x5.h,
arch/arm/include/asm/arch-at91/at91sam9rl.h and
arch/arm/include/asm/arch-at91/at91sam9rl.h ?

And you would add yet another copy of the same?

This is not acceptable.  Please factor these out into a common header
file, so we have only a single copy of these defintiions.

> --- a/arch/arm/include/asm/arch-at91/at91sam9x5_matrix.h
> +++ b/arch/arm/include/asm/arch-at91/at91sam9x5_matrix.h
> @@ -1,10 +1,10 @@
>  /*
>   * Matrix-centric header file for the AT91SAM9X5 family
>   *
> - *  Copyright (C) 2012 Atmel Corporation.
> + *  Copyright (C) 2013 Atmel Corporation.

You cannot and you must not revert existing copyrights.  This must
NEVER be done.  Instead, update it like that:

	Copyright (C) 2012-2013 Atmel Corporation.

> --- a/arch/arm/include/asm/arch-at91/hardware.h
> +++ b/arch/arm/include/asm/arch-at91/hardware.h
> @@ -39,6 +39,8 @@
>  # include <asm/arch/at91sam9g45.h>
>  #elif defined(CONFIG_AT91SAM9X5)
>  # include <asm/arch/at91sam9x5.h>
> +#elif defined(CONFIG_AT91SAM9N12)
> +# include <asm/arch/at91sam9n12.h>
>  #elif defined(CONFIG_AT91CAP9)
>  # include <asm/arch/at91cap9.h>
>  #elif defined(CONFIG_AT91X40)

Another place where lists should be sorted.

> +	lcd_printf("%s\n", U_BOOT_VERSION);
> +	lcd_printf("(C) 2013 ATMEL Corp\n");

U-Boot itself is not (C) ATMEL.

> +#undef CONFIG_CMD_IMI
> +#undef CONFIG_CMD_LOADS

Is there any good reason to disable these?

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
I have a very small mind and must live with it.    -- Edsger Dijkstra