[U-Boot] [PATCH 3/3] mpc52xx: add support for the IPEK01 board

Mon Oct 19 14:25:04 CEST 2009

Dear Wolfgang Grandegger,

In message <20091019111913.802418590 at 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 at 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 at 0"
> +#define OF_SOC			"soc5200 at f0000000"
> +#define OF_TBCLK		(bd->bi_busfreq / 4)
> +#define OF_STDOUT_PATH		"/soc5200 at f0000000/serial at 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: wd at denx.de
How does a project get to be a year late?      ... One day at a time.