[U-Boot] [PATCH 2/2 v3] arm: suen3, suen3_v1, mgcoge2_arm_p1a support
Heiko Schocher
hs at denx.de
Wed Feb 3 16:52:05 CET 2010
Hello Prafulla,
Prafulla Wadaskar wrote:
> First of all sorry for delayed feedback
No problem, thanks for reviewing!
>> -----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
Yep, you are right! I fix this.
>> 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.
This question also asked Tom, see:
http://lists.denx.de/pipermail/u-boot/2010-January/067182.html
But if you prefer to split this in 5 patches, I can do it.
> ...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
Hmm.. this is common code for all keymile boards,
but some architecture specific ifdefs are needed.
I try to factor out the architecture specific
code, and move it to 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?.
Stefan, can you answer this?
>> +}
>> +
>> +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.
OK, I add some comment.
>> +
>> + /*
>> + * 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?
With it, the nand subsystem knows, that there is no longer
the nand availiable.
>> + } 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
OK, move it to the top of the 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
You are right, this could be interesting for others,
so the question is where to put this ... in common/cmd_eeprom.c?
>> +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?
Ok, you are right, some comments are here necessary.
On this boards is a preloader, which initializes
the RAM. Therefore the preloader reads the RAM settings
from the image he should load, through an header. This
header is created with a tool doimage (I think it is
from marvell), and this tool needs this file ...
So, I have no idea where to put this files, and think
they are in the board directory on the right place ...
I found something similiar in current mainline:
board/Marvell/mv88f6281gtw_ge/dramregs_333h.txt
This file is also without comments, license info ...
Maybe this tool don;t accept comments?
>> +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...
see above ...
>> +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..
This should be Ok, as this file collect common config option ...
bye
Heiko
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
More information about the U-Boot
mailing list