Dear Jean-Christophe PLAGNIOL-VILLARD,
In message 1233785700-31051-1-git-send-email-plagnioj@jcrosoft.com you wrote:
From: Ilko Iliev iliev@ronetix.at
This patch adds support for the PM9263 board of Ronetix GmbH (www.ronetix.at) With the necessary to boot the board from NOR
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
I cannot parse this. What do you want to express?
index ec6ad5d..122e23a 100644 --- a/cpu/arm926ejs/at91/lowlevel_init.S +++ b/board/ronetix/pm9263/led.c
NAK.
I will not accept yet another copy of this file. Please create common code that can be configured as needed.
diff --git a/board/ronetix/pm9263/partition.c b/board/ronetix/pm9263/partition.c new file mode 100644 index 0000000..95ac398 --- /dev/null +++ b/board/ronetix/pm9263/partition.c
NAK.
I will not accept yet another copy of this file. We have already 7 of these, most of them identical, and the remaining differences minimal. Please create common code that can be configured as needed.
diff --git a/board/ronetix/pm9263/pm9263.c b/board/ronetix/pm9263/pm9263.c new file mode 100644 index 0000000..7bda1b8
...
+/* ------------------------------------------------------------------------- */ +/*
- Miscelaneous platform dependent initialisations
- */
+static void pm9263_serial_hw_init(void) +{ +#ifdef CONFIG_USART0
- at91_set_A_periph(AT91_PIN_PA26, 1); /* TXD0 */
- at91_set_A_periph(AT91_PIN_PA27, 0); /* RXD0 */
- at91_sys_write(AT91_PMC_PCER, 1 << AT91_ID_US0);
+#endif
+#ifdef CONFIG_USART1
- at91_set_A_periph(AT91_PIN_PD0, 1); /* TXD1 */
- at91_set_A_periph(AT91_PIN_PD1, 0); /* RXD1 */
- at91_sys_write(AT91_PMC_PCER, 1 << AT91_ID_US1);
+#endif
+#ifdef CONFIG_USART2
- at91_set_A_periph(AT91_PIN_PD2, 1); /* TXD2 */
- at91_set_A_periph(AT91_PIN_PD3, 0); /* RXD2 */
- at91_sys_write(AT91_PMC_PCER, 1 << AT91_ID_US2);
+#endif
+#ifdef CONFIG_USART3 /* DBGU */
- at91_set_A_periph(AT91_PIN_PC30, 0); /* DRXD */
- at91_set_A_periph(AT91_PIN_PC31, 1); /* DTXD */
- at91_sys_write(AT91_PMC_PCER, 1 << AT91_ID_SYS);
+#endif +}
I think I've seent his code a couple of times before, too. Last time in another patch earlier today. Seems there are at least 8 files in U-Boot that contain such a sequence, with no or just minimal differences.
Should we not clean this up, too?
+static int pm9263_lcd_hw_psram_init(void) +{
...
- if ((readw(PHYS_PSRAM) != 0x1234) || (readw(PHYS_PSRAM+2) != 0x5678))
- {
Incorrect brace style.
writew(0x5678, PHYS_PSRAM+2);if ((readw(PHYS_PSRAM) != 0x1234)|| (readw(PHYS_PSRAM + 2) != 0x5678))return 1;
I recommend to use curly braces because of the multi-line "if".
Also, please put the "||" at the end of line 1.
+int dram_init(void) +{
- gd->bd->bi_dram[0].start = PHYS_SDRAM;
- gd->bd->bi_dram[0].size = PHYS_SDRAM_SIZE;
- return 0;
+}
You should not hard-wire the memory size. You should auto-size it.
+#ifdef CONFIG_RESET_PHY_R +void reset_phy(void) +{ +#ifdef CONFIG_MACB
- /*
* Initialize ethernet HW addr prior to starting Linux,* needed for nfsroot*/- eth_init(gd->bd);
+#endif
Be careful. You mustnot initialize the ehternet port unless a U-Boot network command is being used. And you must shut it down after that.
+#ifdef CONFIG_DISPLAY_BOARDINFO +int checkboard (void) +{
- char *ss;
- uint32_t reg, diva, mula;
- uint32_t val, mdiv, pres, crystal;
- char buf[32];
- reg = at91_sys_read(AT91_CKGR_PLLAR);
- mula = ((reg >> 16) & 0x7FF) + 1;
- diva = reg & 0xFF;
- reg = at91_sys_read(AT91_PMC_MCKR);
- mdiv = (reg >> 8) & 3;
- pres = (reg >> 2) & 7;
Maybe a comment would help to explain what this code is doing?
- printf("Video memory : 0x%08lX %s\n", gd->fb_base, ss );
No space before the paren.
- printf ("\n");
Please use consistent style: either
printf(...); or printf (...);
bot on;t mix these (this applies to all function calls).
diff --git a/cpu/arm926ejs/at91/lowlevel_init.S b/board/ronetix/pm9263/u-boot.lds similarity index 54% copy from cpu/arm926ejs/at91/lowlevel_init.S copy to board/ronetix/pm9263/u-boot.lds index ec6ad5d..443f180 100644 --- a/cpu/arm926ejs/at91/lowlevel_init.S +++ b/board/ronetix/pm9263/u-boot.lds
That's a pretty funny diff, isn't it? lowlevel_init.S versus u-boot.lds ...
diff --git a/include/configs/pm9263.h b/include/configs/pm9263.h new file mode 100644 index 0000000..4a20bff --- /dev/null +++ b/include/configs/pm9263.h
...
+#define AT91_MAIN_CLOCK (PM9263_CRYSTAL / MASTER_PLL_DIV * MASTER_PLL_MUL)
Maximum line lenght exceeded. Check all files, there are more places like this one!
+#define AT91_MASTER_CLOCK (AT91_MAIN_CLOCK / MAIN_PLL_DIV) +#define CONFIG_SYS_AT91_PLLB 0x10483E0E /* PLLB settings for USB */ +#define CONFIG_SYS_HZ 1000000
NAK.
CONFIG_SYS_HZ must be 1000.
+#define CFG_MONITOR_BASE (CFG_DATAFLASH_LOGIC_ADDR_CS0 + 0x8400) +#define CONFIG_ENV_OFFSET 0x4200 +#define CONFIG_ENV_ADDR (CFG_DATAFLASH_LOGIC_ADDR_CS0 + CONFIG_ENV_OFFSET) +#define CONFIG_ENV_SIZE 0x4200 +#define CONFIG_BOOTCOMMAND "cp.b 0xC0042000 0x22000000 0x210000; bootm"
No need for 0x in U-Boot commands. This is just wasting environment memory.
+#define CONFIG_BOOTCOMMAND "nand read 0x22000000 0xA0000 0x200000; bootm"
Ditto.
+#define ROUND(A, B) (((A) + (B)) & ~((B) - 1))
Do we really need this redefined in each and every include/configs/at91*.h file?
- Size of malloc() pool
- */
+#define CONFIG_SYS_MALLOC_LEN ROUND(3 * CONFIG_ENV_SIZE + 128*1024, 0x1000)
Same here?
Best regards,
Wolfgang Denk