[U-Boot] [PATCH v5] board support patch for phyCORE-MPC5200B-tiny

[Wolfgang Denk]

Dear Jon Smirl,

In message <20090610141432.11516.60764.stgit at terra> you wrote:
> Add support for the Phytec phyCORE-MPC5200B-tiny. Code originally from Pengutronix.de.
> v5 - Rebased onto u-boot/next. Changed official board name to phyCORE-MPC5200B-tiny.

This 2nd line is a comment and must go *below* the "---" line.

> Signed-off-by: Jon Smirl <jonsmirl at gmail.com>
> ---

Add comments only after this line.

> diff --git a/Makefile b/Makefile
> index 4bf4442..fb036c6 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -696,6 +696,15 @@ MVBC_P_config: unconfig
>  o2dnt_config:	unconfig
>  	@$(MKCONFIG) o2dnt ppc mpc5xxx o2dnt
>  
> +pcm030_config \
> +pcm030_LOWBOOT_config:	unconfig
> +	@ >include/config.h
> +	@[ -z "$(findstring LOWBOOT_,$@)" ] || \
> +		{ echo "TEXT_BASE = 0xFF000000"	>board/phytec/pcm030/config.tmp ; \

"$(obj)" missing, will fail for out-of-tree builds.

> diff --git a/board/phytec/pcm030/config.mk b/board/phytec/pcm030/config.mk
> new file mode 100644
> index 0000000..5d3469c
> --- /dev/null
> +++ b/board/phytec/pcm030/config.mk
> @@ -0,0 +1,42 @@
> +#
> +# (C) Copyright 2003
> +# Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> +#
> +# See file CREDITS for list of people who contributed to this
> +# project.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation; either version 2 of
> +# the License, or (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> +# MA 02111-1307 USA
> +#
> +
> +#
> +# phyCORE-MPC5200B tiny board:
> +#
> +#	Valid values for TEXT_BASE are:
> +#
> +#	0xFFF00000   boot high (standard configuration)
> +#	0xFF000000   boot low
> +#	0x00100000   boot from RAM (for testing only)
> +#
> +
> +sinclude $(TOPDIR)/board/$(BOARDDIR)/config.tmp
> +
> +ifndef TEXT_BASE
> +## Standard: boot high
> +TEXT_BASE = 0xFFF00000
> +endif
> +
> +PLATFORM_CPPFLAGS += -DTEXT_BASE=$(TEXT_BASE) -I$(TOPDIR)/board
> +

Coding Style. No trailing empty lines, please.

> diff --git a/board/phytec/pcm030/mt46v32m16-75.h b/board/phytec/pcm030/mt46v32m16-75.h
> new file mode 100644
> index 0000000..4b501c6
> --- /dev/null
> +++ b/board/phytec/pcm030/mt46v32m16-75.h
> @@ -0,0 +1,54 @@
...
> +/* Settings for XLB = 99 MHz */
> +/*
> +#define SDRAM_MODE	0x008D0000
> +#define SDRAM_EMODE	0x40090000
> +#define SDRAM_CONTROL	0x714b0f00
> +#define SDRAM_CONFIG1	0x63611730
> +#define SDRAM_CONFIG2	0x47670000
> +*/

Please do not add dead code.

> diff --git a/board/phytec/pcm030/pcm030.c b/board/phytec/pcm030/pcm030.c
> new file mode 100644
> index 0000000..34e5245
> --- /dev/null
> +++ b/board/phytec/pcm030/pcm030.c
> @@ -0,0 +1,219 @@
> +/*
> + * (C) Copyright 2003
> + * Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> + *
> + * (C) Copyright 2004
> + * Mark Jonas, Freescale Semiconductor, mark.jonas at motorola.com.
> + *
> + * (C) Copyright 2006
> + * Eric Schumann, Phytec Messtechnik GmbH
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include <common.h>
> +#include <mpc5xxx.h>
> +#include <pci.h>
> +#include <asm-ppc/io.h>
> +
> +#include "mt46v32m16-75.h"
> +
> +#ifndef CONFIG_SYS_RAMBOOT
> +static void sdram_start(int hi_addr)
> +{
> +        volatile struct mpc5xxx_cdm *cdm =
> +                (struct mpc5xxx_cdm *)MPC5XXX_CDM;
> +        volatile struct mpc5xxx_sdram *sdram =
> +                (struct mpc5xxx_sdram *)MPC5XXX_SDRAM;

Coding style: indentation by TAB only. (here and elsewhere).

...
> +phys_size_t initdram(int board_type)
> +{
> +        volatile struct mpc5xxx_mmap_ctl *mm =
> +                (struct mpc5xxx_mmap_ctl *)CONFIG_SYS_MBAR;
> +        volatile struct mpc5xxx_cdm *cdm =
> +                (struct mpc5xxx_cdm *)MPC5XXX_CDM;
> +        volatile struct mpc5xxx_sdram *sdram =
> +                (struct mpc5xxx_sdram *)MPC5XXX_SDRAM;

Incorrect indentation again.

> +	ulong dramsize = 0;
> +	ulong dramsize2 = 0;
> +#ifndef CONFIG_SYS_RAMBOOT
> +	ulong test1, test2;
> +
> +	/* setup SDRAM chip selects */
> +							 /* 256MB at 0x0 */
> +	out_be32 (&mm->sdram0, 0x0000001b);
> +							 /* disabled */
> +	out_be32 (&mm->sdram1, 0x10000000);
> +
> +	/* setup config registers */
> +	out_be32 (&sdram->config1, SDRAM_CONFIG1);
> +	out_be32 (&sdram->config2, SDRAM_CONFIG2);
> +
> +#if defined(SDRAM_DDR) && defined(SDRAM_TAPDELAY)
> +	/* set tap delay */
> +	out_be32 (&cdm->porcfg, SDRAM_TAPDELAY);
> +#endif
> +
> +	/* find RAM size using SDRAM CS0 only */
> +	sdram_start(0);
> +	test1 = get_ram_size((long *) CONFIG_SYS_SDRAM_BASE, 0x10000000);
> +	sdram_start(1);
> +	test2 = get_ram_size((long *) CONFIG_SYS_SDRAM_BASE, 0x10000000);
> +	if (test1 > test2) {
> +		sdram_start(0);
> +		dramsize = test1;
> +	} else
> +		dramsize = test2;
> +
> +	/* memory smaller than 1MB is impossible */
> +	if (dramsize < (1 << 20))
> +		dramsize = 0;
> +
> +	/* set SDRAM CS0 size according to the amount of RAM found */
> +	if (dramsize > 0) {
> +		out_be32 (&mm->sdram0,
> +		    (0x13 + __builtin_ffs(dramsize >> 20) - 1));
> +	} else
> +							/* disabled */
> +		out_be32 (&mm->sdram0, 0);

That's a multiline else branch as is and thus requires braces.

> +#else /* CONFIG_SYS_RAMBOOT */
> +
> +	/* retrieve size of memory connected to SDRAM CS0 */
> +	dramsize = in_be32(&mm->sdram0) & 0xFF;
> +	if (dramsize >= 0x13)
> +		dramsize = (1 << (dramsize - 0x13)) << 20;
> +	else
> +		dramsize = 0;
> +
> +	/* retrieve size of memory connected to SDRAM CS1 */
> +	dramsize2 = in_be32(&mm->sdram1) & 0xFF;
> +	if (dramsize2 >= 0x13)
> +		dramsize2 = (1 << (dramsize2 - 0x13)) << 20;
> +	else
> +		dramsize2 = 0;
> +
> +#endif /* CONFIG_SYS_RAMBOOT */
> +
> +	return dramsize + dramsize2;
> +}
> +
> +int checkboard(void)
> +{
> +	puts("Board: phyCORE-MPC5200B-tiny\n");
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PCI
> +static struct pci_controller hose;
> +
> +extern void pci_mpc5xxx_init(struct pci_controller *);
> +
> +void pci_init_board(void)
> +{
> +	pci_mpc5xxx_init(&hose);
> +}
> +#endif
> +
> +#if defined(CONFIG_OF_LIBFDT) && defined(CONFIG_OF_BOARD_SETUP)
> +void ft_board_setup(void *blob, bd_t * bd)
> +{
> +	ft_cpu_setup(blob, bd);
> +}
> +#endif /* defined(CONFIG_OF_LIBFDT) && defined(CONFIG_OF_BOARD_SETUP) */
> +
> +#if defined(CONFIG_CMD_IDE) && defined(CONFIG_IDE_RESET)
> +
> +#define GPIO_PSC2_4	0x02000000UL
> +
> +void init_ide_reset(void)
> +{
> +        volatile struct mpc5xxx_wu_gpio *wu_gpio =
> +                (struct mpc5xxx_wu_gpio *)MPC5XXX_WU_GPIO;

Incorrect indentation again. Will stop here flagging these.


> +	debug("init_ide_reset\n");
> +
> +	/* Configure PSC2_4 as GPIO output for ATA reset */
> +	setbits_be32(&wu_gpio->enable, GPIO_PSC2_4);
> +	setbits_be32(&wu_gpio->ddr, GPIO_PSC2_4);
> +	/* Deassert reset */
> +	setbits_be32(&wu_gpio->dvo, GPIO_PSC2_4);
> +}
> +
> +void ide_set_reset(int idereset)
> +{
> +        volatile struct mpc5xxx_wu_gpio *wu_gpio =
> +                (struct mpc5xxx_wu_gpio *)MPC5XXX_WU_GPIO;
> +	debug("ide_reset(%d)\n", idereset);
> +
> +	if (idereset) {
> +		clrbits_be32(&wu_gpio->dvo, GPIO_PSC2_4);
> +		/* Make a delay. MPC5200 spec says 25 usec min */
> +		udelay(500000);
> +	} else
> +		setbits_be32(&wu_gpio->dvo, GPIO_PSC2_4);
> +}
> +#endif /* defined(CONFIG_CMD_IDE) && defined(CONFIG_IDE_RESET) */
> +
> diff --git a/cpu/mpc5xxx/ide.c b/cpu/mpc5xxx/ide.c
> index 9e8f29b..0129180 100644
> --- a/cpu/mpc5xxx/ide.c
> +++ b/cpu/mpc5xxx/ide.c
> @@ -45,6 +45,9 @@ int ide_preinit (void)
>  #if defined(CONFIG_SYS_ATA_CS_ON_I2C2)
>  	/* ATA cs0/1 on i2c2 clk/io */
>  	reg = (reg & ~0x03000000ul) | 0x02000000ul;
> +#elif defined(CONFIG_PHYCORE_MPC5200B_TINY)
> +	/* ATA cs0/1 on Timer 0/1 */
> +	reg = (reg & ~0x03000000ul) | 0x03000000ul;
>  #else

Please no board-specific #ifdef's in common code. Chose a
feature-based name instead.

> diff --git a/include/configs/pcm030.h b/include/configs/pcm030.h
> new file mode 100644
> index 0000000..6ca7778
> --- /dev/null
> +++ b/include/configs/pcm030.h
> @@ -0,0 +1,472 @@
...
> +#ifndef __CONFIG_H
> +#define __CONFIG_H
> +
> +/* #define DEBUG */
> +
> +/* To build RAMBOOT, replace this section the main Makefile
> +pcm030_config \
> +pcm030_RAMBOOT_config \
> +pcm030_LOWBOOT_config:	unconfig
> +	@ >include/config.h
> +	@[ -z "$(findstring LOWBOOT_,$@)" ] || \
> +		{ echo "TEXT_BASE = 0xFF000000"	>board/phytec/pcm030/config.tmp ; \
> +		  echo "... with LOWBOOT configuration" ; \
> +		}
> +	@[ -z "$(findstring RAMBOOT_,$@)" ] || \
> +	       { echo "TEXT_BASE = 0x00100000" >board/phycore_mpc5200b_tiny/\
> +			config.tmp ; \
> +		 echo "... with RAMBOOT configuration" ; \
> +		 echo "... remember to make sure that MBAR is already \
> +				switched to 0xF0000000 !!!" ; \
> +	       }
> +	@$(MKCONFIG) -a pcm030 ppc mpc5xxx pcm030 phytec
> +	@ echo "remember to set pcm030_REV to 0 for rev 1245.0 rev or to 1 for rev 1245.1"
> +*/

Incorrect multiline comment style. Also, the board config file is not
a place for board documentation. Please add docs in the doc/
directory, or in your board directory.

> +#define CONFIG_SYS_FLASH_BASE		0xff000000
> +#define CONFIG_SYS_FLASH_SIZE		0x01000000

You should rather use auto-sizing (ditto for RAM).

> +/* pcm030 ships with environment is EEPROM by default */
> +#define CONFIG_ENV_IS_IN_EEPROM	1
> +#define CONFIG_ENV_OFFSET	0x00	/* environment starts at the */
> +					/*beginning of the EEPROM */
> +#define CONFIG_ENV_SIZE		CONFIG_SYS_EEPROM_SIZE
> +
> +/* Moving the environment to flash can be more reliable
> +#define CONFIG_ENV_IS_IN_FLASH	1
> +#define CONFIG_ENV_ADDR		(CONFIG_SYS_FLASH_BASE + 0xfe0000)
> +#define CONFIG_ENV_SIZE		0x20000
> +#define CONFIG_ENV_SECT_SIZE	0x20000
> +*/

Incorrect multiline comment style.

And please do not add dead code.


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
The use of COBOL cripples the mind; its teaching  should,  therefore,
be regarded as a criminal offense.                   - E. W. Dijkstra