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

[Josh Wu <josh.wu@atmel.com>]

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@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