Dear Josh Wu,
In message <1363342624-2939-1-git-send-email-josh...@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: w...@denx.de
I have a very small mind and must live with it. -- Edsger Dijkstra
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
