Dear Wolfgang Grandegger,
In message 20091019111913.802418590@denx.de you wrote:
This patch adds support for the board IPEK01 based on the MPC5200. The Futjitsu Lime graphics controller is configured in 16 bpp mode.
Signed-off-by: Wolfgang Grandegger wg@grandegger.com
Makefile | 3 board/ipek01/Makefile | 50 ++++ board/ipek01/config.mk | 30 ++ board/ipek01/ipek01.c | 374 ++++++++++++++++++++++++++++++++++++ board/ipek01/mt46v16m16-75.h | 37 +++ board/ipek01/mt48lc16m16a2-75.h | 43 ++++ board/ipek01/u-boot.lds | 125 ++++++++++++ include/configs/ipek01.h | 411 ++++++++++++++++++++++++++++++++++++++++ 8 files changed, 1073 insertions(+) create mode 100644 board/ipek01/Makefile create mode 100644 board/ipek01/config.mk create mode 100644 board/ipek01/ipek01.c create mode 100644 board/ipek01/mt46v16m16-75.h create mode 100644 board/ipek01/mt48lc16m16a2-75.h create mode 100644 board/ipek01/u-boot.lds create mode 100644 include/configs/ipek01.h
Entries to MAKEALL and MAINTAINERS are missing.
Index: u-boot-mainline/Makefile
--- u-boot-mainline.orig/Makefile 2009-10-19 13:17:12.185302922 +0200 +++ u-boot-mainline/Makefile 2009-10-19 13:17:17.519552635 +0200 @@ -725,6 +725,9 @@ } @$(MKCONFIG) -a PM520 ppc mpc5xxx pm520
+ipek01_config: unconfig
- @$(MKCONFIG) -a ipek01 ppc mpc5xxx ipek01
Please keep list ssorted.
Index: u-boot-mainline/board/ipek01/ipek01.c
--- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ u-boot-mainline/board/ipek01/ipek01.c 2009-10-19 13:17:17.527552105 +0200
...
+#ifndef CONFIG_SYS_RAMBOOT
Is RAM-Boot actually a asupported mode of operation on this board, or are you just copuy & pasting dead code here?
+static void sdram_start (int hi_addr) +{
- long hi_addr_bit = hi_addr ? 0x01000000 : 0;
- /* unlock mode register */
- *(vu_long *) MPC5XXX_SDRAM_CTRL =
SDRAM_CONTROL | 0x80000000 | hi_addr_bit;- __asm__ volatile ("sync");
Please use I/O accessors to write device registers (please fix globally).
...
+phys_size_t initdram (int board_type) +{
- ulong dramsize = 0;
- ulong dramsize2 = 0;
- uint svr, pvr;
+#ifndef CONFIG_SYS_RAMBOOT
- ulong test1, test2;
- /* setup SDRAM chip selects */
- *(vu_long *) MPC5XXX_SDRAM_CS0CFG = 0x0000001e; /* 2G at 0x0 */
- *(vu_long *) MPC5XXX_SDRAM_CS1CFG = 0x00000000; /* disabled */
It might make sense to #define some constants in the board config file.
- /* setup config registers */
- *(vu_long *) MPC5XXX_SDRAM_CONFIG1 = SDRAM_CONFIG1;
- *(vu_long *) MPC5XXX_SDRAM_CONFIG2 = SDRAM_CONFIG2;
- __asm__ volatile ("sync");
+#if SDRAM_DDR
I consider this bad style. In U-Boot, we usually use #ifdef, but this collides with your #define SDRAM_DDR 0 versus 1 below. I recommend to clean this up.
- /*
* On MPC5200B we need to set the special configuration delay in the* DDR controller. Please refer to Freescale's AN3221 "MPC5200B SDRAM* Initialization and Configuration", 3.3.1 SDelay--MBAR + 0x0190:** "The SDelay should be written to a value of 0x00000004. It is* required to account for changes caused by normal wafer processing* parameters."*/- svr = get_svr ();
- pvr = get_pvr ();
- if ((SVR_MJREV (svr) >= 2) && (PVR_MAJ (pvr) == 1) &&
(PVR_MIN (pvr) == 4)) {*(vu_long *) MPC5XXX_SDRAM_SDELAY = 0x04;__asm__ volatile ("sync");- }
Is this test really needed? Are there versions of this board with pre-Rev. B silicon?
+#if defined (CONFIG_CMD_IDE) && defined (CONFIG_IDE_RESET)
+void init_ide_reset (void) +{
- debug ("init_ide_reset\n");
+}
+void ide_set_reset (int idereset) +{
- debug ("ide_reset(%d)\n", idereset);
+} +#endif /* defined (CONFIG_SYS_CMD_IDE) && defined (CONFIG_IDE_RESET) */
This is just dead code. Please get rid of it.
+#ifdef CONFIG_VIDEO +#define DISPLAY_WIDTH 800 +#define DISPLAY_HEIGHT 600
This should go to the board config file.
+#define CONFIG_SYS_LIME_SRST (CONFIG_SYS_LIME_BASE + 0x01FC002C) +#define CONFIG_SYS_LIME_CCF (CONFIG_SYS_LIME_BASE + 0x01FC0038) +#define CONFIG_SYS_LIME_MMR (CONFIG_SYS_LIME_BASE + 0x01FCFFFC) +/* Lime clock frequency */ +#define CONFIG_SYS_LIME_CLK 0x90000 /* geo 166MHz other 133MHz */ +/* SDRAM parameter */ +#define CONFIG_SYS_LIME_MMR_VALUE 0x41c767e3
Please do not use base register plus offset. Use a proper C struct to describe the device registers.
+#define CONFIG_SYS_LIME_CID (CONFIG_SYS_LIME_BASE + 0x01FC00F0) +#define CONFIG_SYS_LIME_REV (CONFIG_SYS_LIME_BASE + 0x01FF8084)
Ditto.
+int lime_probe (void) +{
- uint reg;
- /* Try to access GDC ID/Revision registers */
- reg = in_be32 ((void *)CONFIG_SYS_LIME_CID);
- reg = in_be32 ((void *)CONFIG_SYS_LIME_CID);
- if (reg == 0x303) {
reg = in_be32 ((void *)CONFIG_SYS_LIME_REV);reg = in_be32 ((void *)CONFIG_SYS_LIME_REV);reg = ((reg & ~0xff) == 0x20050100) ? 1 : 0;
Please see above - using a proper C struct we can get rid of these casts.
+#if defined(CONFIG_CONSOLE_EXTRA_INFO) +/*
- Return text to be printed besides the logo.
- */
+void video_get_info_str (int line_number, char *info) +{
- if (line_number == 1) {
strcpy (info, " Board: IPEK01");- } else {
info[0] = '\0';- }
Please use TABs for indentation. And no braces are needed here.
Index: u-boot-mainline/board/ipek01/mt46v16m16-75.h
--- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ u-boot-mainline/board/ipek01/mt46v16m16-75.h 2009-10-19 13:17:17.529553509 +0200
Seems we are adding the 8th copy of this file?
Can you please move this to a common place? Thanks.
+#define SDRAM_DDR 1 /* is DDR */
Dangerous. See somment above.
Index: u-boot-mainline/board/ipek01/mt48lc16m16a2-75.h
--- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ u-boot-mainline/board/ipek01/mt48lc16m16a2-75.h 2009-10-19 13:17:17.530552255 +0200
Seems we are adding the 9th copy of this file?
Can you please move this to a common place? Thanks.
+#define SDRAM_DDR 0 /* is SDR */
Very dangerous. See somment above.
Index: u-boot-mainline/board/ipek01/u-boot.lds
--- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ u-boot-mainline/board/ipek01/u-boot.lds 2009-10-19 13:17:17.540334101 +0200
Does this board really need a private linker script? I don't think so.
Index: u-boot-mainline/include/configs/ipek01.h
--- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ u-boot-mainline/include/configs/ipek01.h 2009-10-19 13:17:17.541552432 +0200
...
+#define CONFIG_MPC5200_DDR 1 /* ... use DDR RAM */
Seems to be redundant with the SDRAM_DDR above. Please decide for one solution.
+#define CONFIG_CMD_DATE /* support for RTC, date/time...*/ +#define CONFIG_CMD_DHCP /* DHCP Support */ +#define CONFIG_CMD_FAT /* FAT support */ +#define CONFIG_CMD_I2C /* I2C serial bus support */ +#define CONFIG_CMD_IRQ /* irqinfo */ +#define CONFIG_CMD_IDE /* IDE harddisk support */ +#define CONFIG_CMD_MII /* MII support */ +#define CONFIG_CMD_PCI /* pciinfo */ +#define CONFIG_CMD_USB /* USB Support */
Maybe you could sort this list? Thanks.
+#if (TEXT_BASE == 0xFC000000) /* Boot low */ +# define CONFIG_SYS_LOWBOOT 1 +#endif
Needed or dead code?
- "rootpath=/opt/eldk41/ppc_6xx\0" \
Is this intentional? Or did you just forget to s/41// ?
- "loadaddr=300000\0" \
This is pretty low; be careful when your kernel grows a bit...
+#define OF_CPU "PowerPC,5200@0" +#define OF_SOC "soc5200@f0000000" +#define OF_TBCLK (bd->bi_busfreq / 4) +#define OF_STDOUT_PATH "/soc5200@f0000000/serial@2000"
Is all this really needed?
+/* Disk-On-Chip currently not supported by U-Boot */ +#define CONFIG_SYS_DOC_BASE 0xE0000000 +#define CONFIG_SYS_DOC_SIZE 0x00100000
Dead code? Please remove.
+#define CONFIG_SYS_INIT_RAM_END MPC5XXX_SRAM_SIZE /* End of used area in DPRAM */
Line too long. There are more too long lines in this patch. Please check globally.
+#undef CONFIG_IDE_8xx_PCCARD /* Use IDE with PC Card Adapter */
+#undef CONFIG_IDE_8xx_DIRECT /* Direct IDE not supported */ +#undef CONFIG_IDE_LED /* LED for ide not supported */
No need to #undef what is not defined anyway.
Please clean up and resubmit. Thanks.
Best regards,
Wolfgang Denk