Dear Wolfgang Grandegger,
In message <20091019111913.802418...@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 <w...@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,5...@0"
> +#define OF_SOC "soc5...@f0000000"
> +#define OF_TBCLK (bd->bi_busfreq / 4)
> +#define OF_STDOUT_PATH "/soc5...@f0000000/ser...@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
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
How does a project get to be a year late? ... One day at a time.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
