[U-Boot] [PATCH 2/2 v3] arm: suen3, suen3_v1, mgcoge2_arm_p1a support

[Prafulla Wadaskar]

Hi Heiko

First of all sorry for delayed feedback


> -----Original Message-----
> From: Heiko Schocher [mailto:hs at denx.de] 
> Sent: Monday, February 01, 2010 1:07 PM
> To: U-Boot user list
> Cc: Wolfgang Denk; Prafulla Wadaskar; Tom
> Subject: [PATCH 2/2 v3] arm: suen3, suen3_v1, mgcoge2_arm_p1a support
> 
> This patch adds support for the Keymile SUEN3 board variants which
> are based on the Marvell Kirkwood (88F6281) SoC. All variants
> uses common code stored in board/keymile/km_arm/km_arm.c
> 
> mgcoge2_arm_p1a board:
> This adds support for the ARM part of the mgcoge2. The suen3
> target was moved to the correct suen3 p1b version. There is a
> difference between the GPIO configuration between suen3 and mgcoge2.
> 
> Signed-off-by: Holger Brunck <holger.brunck at keymile.com>
> Signed-off-by: Stefan Roese <sr at denx.de>
> Signed-off-by: Heiko Schocher <hs at denx.de>
> ---
> - changes since v1:
>   added comments from Wolfgang Denk:
>   get rid of flash_info_t define in board config
>   (to get this working patch 1/2 is introduced/needed)
> - changes since v2:
>   added comments from Wolfgang Denk
>   - rearranged if/else in do_spi_toggle()
>   - added I/O accessor functions for bootcounter
> 
>  MAINTAINERS                           |    2 +
>  MAKEALL                               |    4 +
>  Makefile                              |    8 +
>  board/keymile/common/common.c         |   23 ++-
>  board/keymile/km_arm/Makefile         |   51 +++++
>  board/keymile/km_arm/config.mk        |   25 +++
>  board/keymile/km_arm/km_arm.c         |  343 
> +++++++++++++++++++++++++++++++++
>  board/keymile/km_arm/sdramregs.txt    |   31 +++
>  board/keymile/km_arm/sdramregs_v1.txt |   28 +++
>  include/configs/km-arm.h              |  189 ++++++++++++++++++

The associated board name and c file is named as kw_arm, pls sync on this
This file name should be km_arm.h

>  include/configs/mgcoge2_arm_p1a.h     |   96 +++++++++
>  include/configs/suen3.h               |  103 ++++++++++
>  include/configs/suen3_p1a.h           |   82 ++++++++
>  include/configs/suen3_p1b_p1c.h       |  110 +++++++++++
...snip...

the include/config files indicates that there are five board supports.
Please provide one patch for each board, may be first will be master one.

...snip...
> diff --git a/board/keymile/common/common.c 
> b/board/keymile/common/common.c
> index ec27bda..33857c7 100644
> --- a/board/keymile/common/common.c
> +++ b/board/keymile/common/common.c
> @@ -35,6 +35,7 @@
>  #include <libfdt.h>
>  #endif
> 
> +#include "../common/common.h"
>  #if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C)
>  #include <i2c.h>
> 
> @@ -395,7 +396,12 @@ static void setports (int gpio)
>  #endif
>  #endif
> 
> -#if defined(CONFIG_KM8XX)
> +#if (defined(CONFIG_MACH_SUEN3) && defined(CONFIG_HARD_I2C))
> +#error I2C bus resetsequence not implemented yet.
> +#endif
> +
> +#if defined(CONFIG_KM8XX) || \
> +	(defined(CONFIG_MACH_SUEN3) && defined(CONFIG_SOFT_I2C))
>  static void set_sda (int state)
>  {
>  	I2C_SDA(state);
> @@ -411,6 +417,12 @@ static int get_sda (void)
>  	return I2C_READ;
>  }
> 
> +#if defined(CONFIG_MACH_SUEN3)
> +static int get_scl (void)
> +{
> +	return (kw_gpio_get_value(SUEN3_SCL_PIN) ? 1 : 0);
> +}
> +#else
>  static int get_scl (void)
>  {
>  	int	val;
> @@ -421,10 +433,11 @@ static int get_scl (void)
> 
>  	return ((val & SCL_BIT) == SCL_BIT);
>  }
> +#endif
> 
>  #endif
> 
> -#if !defined(CONFIG_KMETER1)
> +#if !defined(CONFIG_KMETER1) && !defined(CONFIG_SUVD3)
>  static void writeStartSeq (void)
>  {
>  	set_sda (1);
> @@ -483,7 +496,7 @@ static int i2c_make_abort (void)
>   */
>  void i2c_init_board(void)
>  {
> -#if defined(CONFIG_KMETER1)
> +#if defined(CONFIG_KMETER1) || defined(CONFIG_SUVD3)
>  	struct fsl_i2c *dev;
>  	dev = (struct fsl_i2c *) (CONFIG_SYS_IMMR + 
> CONFIG_SYS_I2C_OFFSET);
>  	uchar	dummy;
> @@ -500,7 +513,7 @@ void i2c_init_board(void)
>  	out_8 (&dev->cr, (I2C_CR_MEN));
> 
>  #else
> -#if defined(CONFIG_HARD_I2C)
> +#if defined(CONFIG_HARD_I2C) && !defined(CONFIG_MACH_SUEN3)
>  	volatile immap_t *immap = (immap_t *)CONFIG_SYS_IMMR ;
>  	volatile i2c8260_t *i2c	= (i2c8260_t *)&immap->im_i2c;
> 
> @@ -578,10 +591,12 @@ int fdt_get_node_and_value (void *blob,
>  }
>  #endif
> 
> +#if !defined(CONFIG_MACH_SUEN3)
>  int ethernet_present (void)
>  {
>  	return (in_8((u8 *)CONFIG_SYS_PIGGY_BASE + 
> CONFIG_SYS_SLOT_ID_OFF) & 0x80);
>  }
> +#endif
> 

In general common file should have code common to all, but the patch for this file looks like the code is ifdefed for specific boards. You can keep such code in km_arm.c

>  int board_eth_init (bd_t *bis)
>  {
> diff --git a/board/keymile/km_arm/km_arm.c 
> b/board/keymile/km_arm/km_arm.c
> new file mode 100644
> index 0000000..0edf3b2
> --- /dev/null
> +++ b/board/keymile/km_arm/km_arm.c
> @@ -0,0 +1,343 @@
> +/*
> + * (C) Copyright 2009
> + * Marvell Semiconductor <www.marvell.com>
> + * Prafulla Wadaskar <prafulla at marvell.com>
> + *
> + * (C) Copyright 2009
> + * Stefan Roese, DENX Software Engineering, sr 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., 51 Franklin Street, Fifth Floor, Boston,
> + * MA 02110-1301 USA
> + */
...snip...

> +
> +	str = getenv("mach_type");
> +	if (str != NULL) {
> +		mach_type = simple_strtoul(str, NULL, 10);
> +		printf("Overwriting MACH_TYPE with %d!!!\n", mach_type);
> +		gd->bd->bi_arch_number = mach_type;
> +	}
> +	return 0;

Pls avoid this, the machine types should be predefined and registered first for the board support that you are adding.
Why do you need this in environment?.

> +}
> +
> +int board_init(void)
> +{
> +	u32 tmp;
> +
> +	kirkwood_mpp_conf(kwmpp_config);
> +
> +	tmp = readl(KW_GPIO0_BASE);
> +	writel(tmp | FLASH_GPIO_PIN , KW_GPIO0_BASE);
> +	tmp = readl(KW_GPIO0_BASE + 4);
> +	writel(tmp & (~FLASH_GPIO_PIN) , KW_GPIO0_BASE + 4);
> +	printf("KM: setting NAND mode\n");

Any explaination about what you are doing above? Any comments will help.

> +
> +	/*
> +	 * arch number of board
> +	 */
> +	gd->bd->bi_arch_number = MACH_TYPE_SUEN3;
> +
> +	/* address of boot parameters */
> +	gd->bd->bi_boot_params = kw_sdram_bar(0) + 0x100;
> +
> +#if defined(CONFIG_KIRKWOOD_GPIO)
> +	/* init the GPIO for I2C Bitbang driver */
> +	kw_gpio_set_valid(SUEN3_SDA_PIN, 1);
> +	kw_gpio_set_valid(SUEN3_SCL_PIN, 1);
> +	kw_gpio_direction_output(SUEN3_SDA_PIN, 0);
> +	kw_gpio_direction_output(SUEN3_SCL_PIN, 0);
> +#endif
> +	return 0;
> +}
> +
> +#if defined(CONFIG_CMD_SF)
> +int do_spi_toggle(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> +{
> +	u32 tmp;
> +	if (argc < 2) {
> +		cmd_usage(cmdtp);
> +		return 1;
> +	}
> +
> +	if ((strcmp(argv[1], "off") == 0)) {
> +		printf("SPI FLASH disabled, NAND enabled\n");
> +		/* Multi-Purpose Pins Functionality configuration */
> +		kwmpp_config[0] = MPP0_NF_IO2;
> +		kwmpp_config[1] = MPP1_NF_IO3;
> +		kwmpp_config[2] = MPP2_NF_IO4;
> +		kwmpp_config[3] = MPP3_NF_IO5;
> +
> +		kirkwood_mpp_conf(kwmpp_config);
> +		tmp = readl(KW_GPIO0_BASE);
> +		writel(tmp | FLASH_GPIO_PIN , KW_GPIO0_BASE);
> +
> +		nand_init();
> +	} else if ((strcmp(argv[1], "on") == 0)) {
> +		printf("SPI FLASH enabled, NAND disabled\n");
> +		/* Multi-Purpose Pins Functionality configuration */
> +		kwmpp_config[0] = MPP0_SPI_SCn;
> +		kwmpp_config[1] = MPP1_SPI_MOSI;
> +		kwmpp_config[2] = MPP2_SPI_SCK;
> +		kwmpp_config[3] = MPP3_SPI_MISO;
> +
> +		kirkwood_mpp_conf(kwmpp_config);
> +		tmp = readl(KW_GPIO0_BASE);
> +		writel(tmp & (~FLASH_GPIO_PIN) , KW_GPIO0_BASE);
> +
> +		nand_init();

What do you need nand_init for disabled nand operation?

> +	} else {
> +		cmd_usage(cmdtp);
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +U_BOOT_CMD(
> +	spitoggle,	2,	0,	do_spi_toggle,
> +	"En-/disable SPI FLASH access",
> +	"<on|off> - Enable (on) or disable (off) SPI FLASH access\n"
> +	);
> +#endif
> +
> +int dram_init(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
> +		gd->bd->bi_dram[i].start = kw_sdram_bar(i);
> +		gd->bd->bi_dram[i].size = get_ram_size((long 
> *)kw_sdram_bar(i),
> +						       kw_sdram_bs(i));
> +	}
> +	return 0;
> +}
> +
> +/* Configure and enable MV88E1118 PHY */
> +void reset_phy(void)
> +{
> +	char *name = "egiga0";
> +
> +	if (miiphy_set_current_dev(name))
> +		return;
> +
> +	/* reset the phy */
> +	miiphy_reset(name, CONFIG_PHY_BASE_ADR);
> +}
> +
> +#if defined(CONFIG_HUSH_INIT_VAR)
> +int hush_init_var (void)
> +{
> +	ivm_read_eeprom ();
> +	return 0;
> +}
> +#endif
> +
> +#if defined(CONFIG_BOOTCOUNT_LIMIT)
> +void bootcount_store (ulong a)
> +{
> +	volatile ulong *save_addr;
> +	volatile ulong size = 0;
> +	int i;
> +	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
> +		size += gd->bd->bi_dram[i].size;
> +	}
> +	save_addr = (ulong*)(size - BOOTCOUNT_ADDR);
> +	writel(a, save_addr);
> +	writel(BOOTCOUNT_MAGIC, &save_addr[1]);
> +}
> +
> +ulong bootcount_load (void)
> +{
> +	volatile ulong *save_addr;
> +	volatile ulong size = 0;
> +	int i;
> +	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {
> +		size += gd->bd->bi_dram[i].size;
> +	}
> +	save_addr = (ulong*)(size - BOOTCOUNT_ADDR);
> +	if (readl(&save_addr[1]) != BOOTCOUNT_MAGIC)
> +		return 0;
> +	else
> +		return readl(save_addr);
> +}
> +#endif
> +
> +#if defined(CONFIG_SYS_EEPROM_WREN)
> +#define BOCOADDR	0x10
> +#define DIRECT		0x04
> +#define DIRECTMASK	0x04
> +#define GPRT		0x08
> +#define GPRTMASK	0x04

Better if you provide defination on the top or in header file

> +int eeprom_reg_value (unsigned dev_addr, uchar reg, uchar 
> reg_mask, int value)
> +{
> +	uchar buf;
> +	if (i2c_read (dev_addr, reg, 1, &buf, 1) != 0)
> +		return 1;
> +	if (value)
> +		buf |= reg_mask;
> +	else
> +		buf &= ~reg_mask;
> +	if (i2c_write (dev_addr, reg, 1, &buf, 1) != 0)
> +		return 1;
> +	return 0;
> +}
> +

It would be nice if you can abstract eeprom related code to separate driver or use existing eeprom driver 
Any way keep it out side of board files, that can be shared by others too

> +int eeprom_write_enable (unsigned dev_addr, int state)
> +{
> +	uchar direction = state;
> +	uchar gpio_value = !state;
> +	uchar addr = BOCOADDR;
> +	uchar direct = DIRECT;
> +	uchar direct_mask = DIRECTMASK;
> +	uchar gprt = GPRT;
> +	uchar gprt_mask = GPRTMASK;
> +
> +	/* state 0: transition from write enabled to write disable */
> +	/* state 1: transition from write disabled to write enable */
> +
> +	if (!state && eeprom_reg_value (addr, gprt, gprt_mask, 
> gpio_value))
> +		return 1;
> +
> +	if (eeprom_reg_value (addr, direct, direct_mask, direction))
> +		return 1;
> +
> +	if (state && eeprom_reg_value (addr, gprt, gprt_mask, 
> gpio_value))
> +		return 1;
> +
> +	return 0;
> +}
> +#endif
> diff --git a/board/keymile/km_arm/sdramregs.txt 
> b/board/keymile/km_arm/sdramregs.txt
> new file mode 100644
> index 0000000..68c53a7
> --- /dev/null
> +++ b/board/keymile/km_arm/sdramregs.txt
> @@ -0,0 +1,31 @@

What is this file?
Which license?
Who is using it?

> +0xFFD10000 0x01112222
> +0xFFD10008 0x00001100
> +0xFFD100E0 0x1B1B1B1B
> +0xFFD20134 0xFFFFFFFF
> +0xFFD20138 0x009FFFFF
...snip..
> +0x0 0x0
> +
> diff --git a/board/keymile/km_arm/sdramregs_v1.txt 
> b/board/keymile/km_arm/sdramregs_v1.txt
> new file mode 100644
> index 0000000..6778a9b
> --- /dev/null
> +++ b/board/keymile/km_arm/sdramregs_v1.txt
> @@ -0,0 +1,28 @@

Ditto...

> +0xFFD10000 0x01111111
> +0xFFD10008 0x00001100
..snip...
> +0x0 0x0
...snip...

> diff --git a/include/configs/suen3_p1b_p1c.h 
> b/include/configs/suen3_p1b_p1c.h
> new file mode 100644
> index 0000000..cd8bf01
> --- /dev/null
> +++ b/include/configs/suen3_p1b_p1c.h
> @@ -0,0 +1,110 @@
> +/*
> + * (C) Copyright 2009
> + * Marvell Semiconductor <www.marvell.com>
> + * Prafulla Wadaskar <prafulla at marvell.com>
> + *
> + * (C) Copyright 2009
> + * Stefan Roese, DENX Software Engineering, sr 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., 51 Franklin Street, Fifth Floor, Boston,
> + * MA 02110-1301 USA
> + */
> +
> +/*
> + * for linking errors see
> + * http://lists.denx.de/pipermail/u-boot/2009-July/057350.html
> + */
> +
> +#ifndef _CONFIG_SUEN3_H
> +#define _CONFIG_SUEN3_H
> +
> +/* include common defines/options for all arm based Keymile boards */
> +#include "km-arm.h"

Further includes are not allowed in board config header.
May be Wolfgang can comment on this..

Regards..
Prafulla . .

>