Dear Andreas Bießmann, Remy Bohmer,
>
> I saw your request for review and have some (little) comments which may be 
> pointed out by Reinhard too.

Thank you for helping me out. I have a few further "recommendations":

>
> Am 05.02.2011 um 13:43 schrieb Remy Bohmer:
>
>> Since U-boot v2010.12 the support for the at91sam9261ek board is broken.
>> This patch solves this issue. This change has been tested on this board.
>>
>> Note: It requires that the 1st stage bootloader (like Atmel at91-bootstrap)
>> to load U-boot at a different address compared to previous releases of
>> U-boot due to conflicts in the BSS area during relocation.
>> (0x23f00000 ->  -0x20a00000)

See comment below.

>>
>> Signed-off-by: Remy Bohmer<li...@bohmer.net>
>> ---
>> V3: This patch applied to either mainstream or the u-boot-atmel branch
>>      It contains all changes required to make mainstream work again for
>>      this board.
>>
>>      This patch can be pulled from:
>>      git://git.denx.de/u-boot-usb.git ->  fix-at91sam9261ek
>>
>> board/atmel/at91sam9261ek/Makefile        |    6 +-
>> board/atmel/at91sam9261ek/at91sam9261ek.c |    7 +-
>> board/atmel/at91sam9261ek/config.mk       |    1 -
>> include/configs/at91sam9261ek.h           |  129 ++++++++++++++++------------
>> 4 files changed, 81 insertions(+), 62 deletions(-)
>> delete mode 100644 board/atmel/at91sam9261ek/config.mk
>>
>> diff --git a/board/atmel/at91sam9261ek/Makefile 
>> b/board/atmel/at91sam9261ek/Makefile
>> index 9d20ba0..3351493 100644
>> --- a/board/atmel/at91sam9261ek/Makefile
>> +++ b/board/atmel/at91sam9261ek/Makefile
>
> is OK
>
>> diff --git a/board/atmel/at91sam9261ek/at91sam9261ek.c 
>> b/board/atmel/at91sam9261ek/at91sam9261ek.c
>> index de5cfae..25c181e 100644
>> --- a/board/atmel/at91sam9261ek/at91sam9261ek.c
>> +++ b/board/atmel/at91sam9261ek/at91sam9261ek.c
>
> is OK
>
>> diff --git a/board/atmel/at91sam9261ek/config.mk 
>> b/board/atmel/at91sam9261ek/config.mk
>> deleted file mode 100644
>> index e554a45..0000000
>
>> diff --git a/include/configs/at91sam9261ek.h 
>> b/include/configs/at91sam9261ek.h
>> index 401478b..d601dad 100644
>> --- a/include/configs/at91sam9261ek.h
>> +++ b/include/configs/at91sam9261ek.h
>> @@ -29,34 +29,42 @@
>>
>> #define CONFIG_AT91_LEGACY
>
> will be removed in Patch 2/2, therefore OK
>
>> +/*
>> + * WARNING:
>> + * The initial boot program needs to be adapted such that it loads U-boot
>> + * at the provided TEXT_BASE below. Note that the Atmel AT91-bootstrap 
>> loader
>> + * might be configured such that it loads U-boot at 0x23f00000. But since
>> + * U-boot is now being relocated to the end of RAM, this will result in a
>> + * lockup during boot due to an overlap in the BSS segment. So, we choose a
>> + * safe load adress to begin with, namely 0x20a00000
>> + */
>> +#define CONFIG_SYS_TEXT_BASE                0x20a00000

I'd recommend setting this to the very beginning of SDRAM(0x20000000),
note that initial SP and GD would be put into SRAM further down.

>> +
>> /* ARM asynchronous clock */
>> -#define CONFIG_SYS_AT91_MAIN_CLOCK  18432000        /* 18.432 MHz crystal */
>> -#define CONFIG_SYS_HZ               1000
>> +#define CONFIG_SYS_AT91_SLOW_CLOCK  32768           /* slow clock xtal */
>> +#define CONFIG_SYS_AT91_MAIN_CLOCK  18432000        /* main clock xtal */
>> +#define CONFIG_SYS_HZ                       1000
>>
>> #define CONFIG_ARM926EJS     1       /* This is an ARM926EJS Core    */
>> +/* Define actual evaluation board type from used processor type */
>> #ifdef CONFIG_AT91SAM9G10EK
>> #define CONFIG_AT91SAM9G10   1       /* It's an Atmel AT91SAM9G10 SoC*/
>> #else
>> #define CONFIG_AT91SAM9261   1       /* It's an Atmel AT91SAM9261 SoC*/

That looks to me the other way: its defining the processor type from the
board type. If both boards are identical but for the processor, the processor
should be defined in boards.cfg, like it is for 9260/9xe-ek.

>
> do not define to '1' here
>
>> #endif
>> +#define CONFIG_AT91FAMILY
>> +
>> +/* Misc CPU related */
>> #define CONFIG_ARCH_CPU_INIT
>> #undef CONFIG_USE_IRQ                        /* we don't need IRQ/FIQ stuff  
>> */
>> -
>> -#define CONFIG_CMDLINE_TAG  1       /* enable passing of ATAGs      */
>> -#define CONFIG_SETUP_MEMORY_TAGS 1
>> -#define CONFIG_INITRD_TAG   1
>> +#define CONFIG_CMDLINE_TAG          /* enable passing of ATAGs */
>> +#define CONFIG_SETUP_MEMORY_TAGS
>> +#define CONFIG_INITRD_TAG
>>
>> #define CONFIG_SKIP_LOWLEVEL_INIT
>>
>> -/*
>> - * Hardware drivers
>> - */
>> -#define CONFIG_AT91_GPIO    1
>> -#define CONFIG_ATMEL_USART  1
>> -#undef CONFIG_USART0
>> -#undef CONFIG_USART1
>> -#undef CONFIG_USART2
>> -#define CONFIG_USART3               1       /* USART 3 is DBGU */
>> +/* general purpose I/O */
>> +#define CONFIG_AT91_GPIO
>>
>> /* LCD */
>> #define CONFIG_LCD                   1
>
> no '1' here
>
>> @@ -65,22 +73,31 @@
>> #undef LCD_TEST_PATTERN
>> #define CONFIG_LCD_INFO                      1
>
> no '1' here ... fix globally, only define to an numerical value if that value 
> is used in code, see Reinhard's mail '[STATUS: AT91/AVR32]' point '1.'
>
>> #define CONFIG_LCD_INFO_BELOW_LOGO   1
>> -#define CONFIG_SYS_WHITE_ON_BLACK           1
>> +#define CONFIG_SYS_WHITE_ON_BLACK   1
>> #define CONFIG_ATMEL_LCD             1
>> #ifdef CONFIG_AT91SAM9261EK
>> #define CONFIG_ATMEL_LCD_BGR555              1
>> #else
>> -#define     CONFIG_AT91SAM9G10_LCD_BASE             0x23E00000      /* LCD 
>> is no more in SRAM */
>> +#define     CONFIG_AT91SAM9G10_LCD_BASE     0x23E00000 /* LCD is no more in 
>> SRAM */
>> #endif
>> #define CONFIG_SYS_CONSOLE_IS_IN_ENV         1
>>
>> +/* serial console */
>> +#define CONFIG_ATMEL_USART
>> +#undef CONFIG_USART0
>> +#undef CONFIG_USART1
>> +#undef CONFIG_USART2
>> +#define CONFIG_USART3               1       /* USART 3 is DBGU */

See my "squash" notice below.

>
> do not undef not defiend values, fix '1'
>
>> +#define CONFIG_BAUDRATE                     115200
>> +#define CONFIG_SYS_BAUDRATE_TABLE   {115200 , 19200, 38400, 57600, 9600 }
>> +
>> /* LED */
>> #define CONFIG_AT91_LED
>> -#define     CONFIG_RED_LED          AT91_PIN_PA23   /* this is the power 
>> led */
>> -#define     CONFIG_GREEN_LED        AT91_PIN_PA13   /* this is the user1 
>> led */
>> -#define     CONFIG_YELLOW_LED       AT91_PIN_PA14   /* this is the user2 
>> led */
>> +#define     CONFIG_RED_LED                  AT91_PIN_PA23   /* the power 
>> led */
>> +#define     CONFIG_GREEN_LED                AT91_PIN_PA13   /* the user1 
>> led */
>> +#define     CONFIG_YELLOW_LED               AT91_PIN_PA14   /* the user2 
>> led */
>>
>> -#define CONFIG_BOOTDELAY    3
>> +#define CONFIG_BOOTDELAY            3
>>
>> /*
>>   * BOOTP options
>> @@ -101,21 +118,28 @@
>> #undef CONFIG_CMD_LOADS
>> #undef CONFIG_CMD_SOURCE
>>
>> -#define CONFIG_CMD_PING             1
>> -#define CONFIG_CMD_DHCP             1
>> -#define CONFIG_CMD_NAND             1
>> -#define CONFIG_CMD_USB              1
>> +#define CONFIG_CMD_PING                     1
>> +#define CONFIG_CMD_DHCP                     1
>> +#define CONFIG_CMD_NAND                     1
>> +#define CONFIG_CMD_USB                      1
>> +#define CONFIG_CMD_CACHE            1
>>
>> -/* SDRAM */
>> +/*
>> + * SDRAM: 1 bank, 64 MB
>> + * Initialized before u-boot gets started.
>> + */
>> #define CONFIG_NR_DRAM_BANKS         1
>> -#define PHYS_SDRAM                  0x20000000
>> -#define PHYS_SDRAM_SIZE                     0x04000000      /* 64 megs */
>> +#define CONFIG_SYS_SDRAM_BASE               0x20000000
>> +#define CONFIG_SYS_SDRAM_SIZE               0x04000000 /* 64 megs */

It is up to you, but (64 << 20) might look "nicer".

>>
>> +/* size in bytes reserved for initial data */
>> +#define CONFIG_SYS_INIT_SP_ADDR             (CONFIG_SYS_SDRAM_BASE + 0x1000 
>> \
>> +                                    - GENERATED_GBL_DATA_SIZE)

I've put that into SRAM, that facilitates future NOR flash boot variants where 
SDRAM
would be initialized by low level it...

>> /* DataFlash */
>> #define CONFIG_ATMEL_DATAFLASH_SPI
>> #define CONFIG_HAS_DATAFLASH         1
>> -#define CONFIG_SYS_SPI_WRITE_TOUT           (5*CONFIG_SYS_HZ)
>> -#define CONFIG_SYS_MAX_DATAFLASH_BANKS              2
>> +#define CONFIG_SYS_SPI_WRITE_TOUT   (5*CONFIG_SYS_HZ)
>> +#define CONFIG_SYS_MAX_DATAFLASH_BANKS      2
>> #define CONFIG_SYS_DATAFLASH_LOGIC_ADDR_CS0  0xC0000000      /* CS0 */
>> #define CONFIG_SYS_DATAFLASH_LOGIC_ADDR_CS3  0xD0000000      /* CS3 */
>> #define AT91_SPI_CLK                 15000000
>> @@ -125,20 +149,18 @@
>> /* NAND flash */
>> #ifdef CONFIG_CMD_NAND
>> #define CONFIG_NAND_ATMEL
>> -#define CONFIG_SYS_MAX_NAND_DEVICE          1
>> -#define CONFIG_SYS_NAND_BASE                        0x40000000
>> -#define CONFIG_SYS_NAND_DBW_8                       1
>> -/* our ALE is AD22 */
>> -#define CONFIG_SYS_NAND_MASK_ALE            (1<<  22)
>> -/* our CLE is AD21 */
>> -#define CONFIG_SYS_NAND_MASK_CLE            (1<<  21)
>> -#define CONFIG_SYS_NAND_ENABLE_PIN          AT91_PIN_PC14
>> -#define CONFIG_SYS_NAND_READY_PIN           AT91_PIN_PC15
>> +#define CONFIG_SYS_MAX_NAND_DEVICE  1
>> +#define CONFIG_SYS_NAND_BASE                0x40000000
>> +#define CONFIG_SYS_NAND_DBW_8
>> +#define CONFIG_SYS_NAND_MASK_ALE    (1<<  22)
>> +#define CONFIG_SYS_NAND_MASK_CLE    (1<<  21)
>> +#define CONFIG_SYS_NAND_ENABLE_PIN  AT91_PIN_PC14
>> +#define CONFIG_SYS_NAND_READY_PIN   AT91_PIN_PC15
>>
>> #endif
>>
>> /* NOR flash - no real flash on this board */
>> -#define CONFIG_SYS_NO_FLASH                 1
>> +#define CONFIG_SYS_NO_FLASH         1
>>
>> /* Ethernet */
>> #define CONFIG_NET_MULTI             1
>> @@ -155,21 +177,21 @@
>> #define CONFIG_USB_ATMEL
>> #define CONFIG_USB_OHCI_NEW          1
>> #define CONFIG_DOS_PARTITION         1
>> -#define CONFIG_SYS_USB_OHCI_CPU_INIT                1
>> -#define CONFIG_SYS_USB_OHCI_REGS_BASE               0x00500000      /* 
>> AT91SAM9261_UHP_BASE */
>> +#define CONFIG_SYS_USB_OHCI_CPU_INIT        1
>> +#define CONFIG_SYS_USB_OHCI_REGS_BASE       0x00500000      /* 
>> AT91SAM9261_UHP_BASE */
>> #ifdef CONFIG_AT91SAM9G10EK
>> -#define CONFIG_SYS_USB_OHCI_SLOT_NAME               "at91sam9g10"
>> +#define CONFIG_SYS_USB_OHCI_SLOT_NAME       "at91sam9g10"
>> #else
>> -#define CONFIG_SYS_USB_OHCI_SLOT_NAME               "at91sam9261"
>> +#define CONFIG_SYS_USB_OHCI_SLOT_NAME       "at91sam9261"
>> #endif

Could we not use "ATMEL_CPU_NAME" here? Its capitalized,
but I assume that would not matter?

>> #define CONFIG_SYS_USB_OHCI_MAX_ROOT_PORTS   2
>> #define CONFIG_USB_STORAGE           1
>> #define CONFIG_CMD_FAT                       1
>>
>> -#define CONFIG_SYS_LOAD_ADDR                        0x22000000      /* load 
>> address */
>> +#define CONFIG_SYS_LOAD_ADDR                0x22000000      /* load address 
>> */
>>
>> -#define CONFIG_SYS_MEMTEST_START            PHYS_SDRAM
>> -#define CONFIG_SYS_MEMTEST_END                      0x23e00000
>> +#define CONFIG_SYS_MEMTEST_START    CONFIG_SYS_SDRAM_BASE
>> +#define CONFIG_SYS_MEMTEST_END              0x23e00000
>>
>> #ifdef CONFIG_SYS_USE_DATAFLASH_CS0
>>
>> @@ -177,7 +199,7 @@
>> #define CONFIG_ENV_IS_IN_DATAFLASH   1
>> #define CONFIG_SYS_MONITOR_BASE      (CONFIG_SYS_DATAFLASH_LOGIC_ADDR_CS0 + 
>> 0x8400)
>> #define CONFIG_ENV_OFFSET    0x4200
>> -#define CONFIG_ENV_ADDR             (CONFIG_SYS_DATAFLASH_LOGIC_ADDR_CS0 + 
>> CONFIG_ENV_OFFSET)
>> +#define CONFIG_ENV_ADDR     (CONFIG_SYS_DATAFLASH_LOGIC_ADDR_CS0 + 
>> CONFIG_ENV_OFFSET)
>> #define CONFIG_ENV_SIZE              0x4200
>> #define CONFIG_BOOTCOMMAND   "cp.b 0xC0042000 0x22000000 0x210000; bootm"
>> #define CONFIG_BOOTARGS              "console=ttyS0,115200 "                 
>> \
>> @@ -202,10 +224,10 @@
>> #else /* CONFIG_SYS_USE_NANDFLASH */
>>
>> /* bootstrap + u-boot + env + linux in nandflash */
>> -#define CONFIG_ENV_IS_IN_NAND       1
>> +#define CONFIG_ENV_IS_IN_NAND               1
>> #define CONFIG_ENV_OFFSET            0x60000
>> #define CONFIG_ENV_OFFSET_REDUND     0x80000
>> -#define CONFIG_ENV_SIZE             0x20000         /* 1 sector = 128 kB */
>> +#define CONFIG_ENV_SIZE                     0x20000         /* 1 sector = 
>> 128 kB */
>> #define CONFIG_BOOTCOMMAND   "nand read 0x22000000 0xA0000 0x200000; bootm"
>> #define CONFIG_BOOTARGS              "console=ttyS0,115200 "                 
>> \
>>                              "root=/dev/mtdblock5 "                  \
>> @@ -216,22 +238,19 @@
>>
>> #endif
>>
>> -#define CONFIG_BAUDRATE             115200
>> -#define CONFIG_SYS_BAUDRATE_TABLE   {115200 , 19200, 38400, 57600, 9600 }
>> -
>> #define CONFIG_SYS_PROMPT            "U-Boot>  "
>> #define CONFIG_SYS_CBSIZE            256
>> #define CONFIG_SYS_MAXARGS           16
>> #define CONFIG_SYS_PBSIZE            (CONFIG_SYS_CBSIZE + 
>> sizeof(CONFIG_SYS_PROMPT) + 16)
>> #define CONFIG_SYS_LONGHELP          1
>> -#define CONFIG_CMDLINE_EDITING      1
>> +#define CONFIG_CMDLINE_EDITING              1
>>
>> /*
>>   * Size of malloc() pool
>>   */
>> #define CONFIG_SYS_MALLOC_LEN                ROUND(3 * CONFIG_ENV_SIZE + 
>> 128*1024, 0x1000)
>>
>> -#define CONFIG_STACKSIZE    (32*1024)       /* regular stack */
>> +#define CONFIG_STACKSIZE            (32*1024) /* regular stack */
>>
>> #ifdef CONFIG_USE_IRQ
>> #error CONFIG_USE_IRQ not supported
>
> please merge move of at91sam9261 to boards.cfg from Patch 2/2 here, this 
> patch could IMHO still go into v2011.03; Patch 2/2 are changes for 
> u-boot-atmel/rework2011xxx branch ... Reinhard do you agree?

In theory, yes, it could be considered a "bug fix". But whats the point of this 
double effort? Its easier when
both patches are merged together and applied to top of rework.

Best Regards, Reinhard
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to