From mboxrd@z Thu Jan 1 00:00:00 1970 From: Josh Wu Date: Mon, 18 Mar 2013 17:57:39 +0800 Subject: [U-Boot] [PATCH] arm: at91: at91sam9n12ek: add nandflash/spiflash/mmc/lcd support In-Reply-To: <20130318065821.566A820058D@gemini.denx.de> References: <1363342624-2939-1-git-send-email-josh.wu@atmel.com> <20130318065821.566A820058D@gemini.denx.de> Message-ID: <5146E513.1010009@atmel.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >> #elif defined(CONFIG_AT91SAM9X5) >> # include >> +#elif defined(CONFIG_AT91SAM9N12) >> +# include >> #elif defined(CONFIG_AT91CAP9) >> # include >> #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