[U-Boot] [PATCH] arm: at91: at91sam9n12ek: add nandflash/spiflash/mmc/lcd support
Josh Wu
josh.wu at atmel.com
Mon Mar 18 10:57:39 CET 2013
Dear Wolfgang Denk
Thanks for your review. See my comment below:
On 3/18/2013 2:58 PM, Wolfgang Denk wrote:
> 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)
>
I'll fix them in next version.
>
>> +++ 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).
yes, the at91sam9n12 is a subset of 9x5 with little modification.
I will reuse the at91sam9x5.h for at91sam9n12.
>
> 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 ?
The at91sam9rl.h file are very different with at91sam9x5/9n12.
only a few peripherals has same id and offset(fiq, aic, pmc, dbgu, pio),
all others are different.
so I think I will just keep the at91sam9rl.h not touched.
>
> 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.
like said in above, I will reuse the at91sam9x5.h for 9n12 chip and keep
at91sam9rl.h untouched.
>
>
>> --- 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.
thank you for the reminding. Now I got it.
>
>> --- 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.
sure.
>
>
>> + lcd_printf("%s\n", U_BOOT_VERSION);
>> + lcd_printf("(C) 2013 ATMEL Corp\n");
> U-Boot itself is not (C) ATMEL.
I will remove the copyright (C) 2013, and just keep 'ATMEL Corp' to
indicate that is ATMEL chips and boards, what do you think?
+ lcd_printf("ATMEL Corp\n");
>
>> +#undef CONFIG_CMD_IMI
>> +#undef CONFIG_CMD_LOADS
> Is there any good reason to disable these?
I will keep these two commands.
>
>
> Best regards,
>
> Wolfgang Denk
>
Thanks again.
Best Regards,
Josh Wu
More information about the U-Boot
mailing list