[U-Boot] [PATCH 1/4 v4] arm: add support for the mgcoge2_arm_p1a board from keymile

[Heiko Schocher <hs@denx.de>]

Hello Prafulla,

Prafulla Wadaskar wrote:
>> -----Original Message-----
>> From: Heiko Schocher [mailto:hs at denx.de]
>> Sent: Friday, February 12, 2010 1:36 PM
>> To: U-Boot user list
>> Cc: Wolfgang Denk; Prafulla Wadaskar; Scott Wood; Stefan Roese
>> Subject: [PATCH 1/4 v4] arm: add support for the
>> mgcoge2_arm_p1a board from keymile
>>
>> Add support for the ARM part of the mgcoge2. This board
>> is based on the Marvell Kirkwood (88F6281) SoC. As there
>> come more board variants, common code is stored in
>> board/keymile/km_arm/km_arm.c
>>
>> Signed-off-by: Holger Brunck <holger.brunck@keymile.com>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Signed-off-by: Heiko Schocher <hs@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
>>
>> - changes since v3:
>>   added comment Scott Wood
>>   - removed nand_init in do_spi_toggle()
>>
>>   added comments from Prafulla Wadagaskar
>>   - km-arm.h renamed to km_arm.h
>>   - reworked eeprom_write_enable() (deleted it)
>>     (when reviewing this function, it cropped up, that
>>      this pin is connected through a gpio pin, not as
>>      in previous version, through the boco (a FPGA))
>>   - moved set_sda(), set_scl(), get_sda(), get_scl()
>>     to km_arm.c
>>   - split patch in 4 patches (for each board an extra patch)
>>   - renamed sdramregs.txt in kwbimage.cfg, also license
>>     info added.
>>
>>  MAINTAINERS                       |    2 +
>>  MAKEALL                           |    1 +
>>  Makefile                          |    3 +
>>  board/keymile/common/common.c     |    6 +-
>>  board/keymile/km_arm/Makefile     |   51 ++++++
>>  board/keymile/km_arm/config.mk    |   28 +++
>>  board/keymile/km_arm/km_arm.c     |  339
>> +++++++++++++++++++++++++++++++++++++
>>  board/keymile/km_arm/kwbimage.cfg |   59 +++++++
>>  include/configs/km_arm.h          |  191 +++++++++++++++++++++
>>  include/configs/mgcoge2_arm_p1a.h |   96 +++++++++++
>>  10 files changed, 774 insertions(+), 2 deletions(-)
>>  create mode 100644 board/keymile/km_arm/Makefile
>>  create mode 100644 board/keymile/km_arm/config.mk
>>  create mode 100644 board/keymile/km_arm/km_arm.c
>>  create mode 100644 board/keymile/km_arm/kwbimage.cfg
>>  create mode 100644 include/configs/km_arm.h
>>  create mode 100644 include/configs/mgcoge2_arm_p1a.h
[...]
>> diff --git a/board/keymile/common/common.c
>> b/board/keymile/common/common.c
>> index ec27bda..7b4eefd 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"
> 
> Can't you use "common.h" here ?

No, this is "just" a keymile specific header for including
functions, which are "common" for all keymile boards ...

>>  #if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C)
>>  #include <i2c.h>
>>
>> @@ -421,7 +422,6 @@ static int get_scl (void)
>>
>>       return ((val & SCL_BIT) == SCL_BIT);
>>  }
>> -
>>  #endif
>>
>>  #if !defined(CONFIG_KMETER1)
>> @@ -500,7 +500,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)
> 
> This is not with reference to this board

It is defined in include/configs/km_arm.h line 41

(As I said, this "4" boards are all the same, just different
hardwarerevisions ...)

>>       volatile immap_t *immap = (immap_t *)CONFIG_SYS_IMMR ;
>>       volatile i2c8260_t *i2c = (i2c8260_t *)&immap->im_i2c;
>>
>> @@ -578,10 +578,12 @@ int fdt_get_node_and_value (void *blob,
>>  }
>>  #endif
>>
>> +#if !defined(CONFIG_MACH_SUEN3)
> 
> Ditto

See comment above.

[...]
>> diff --git a/board/keymile/km_arm/km_arm.c
>> b/board/keymile/km_arm/km_arm.c
>> new file mode 100644
>> index 0000000..e1cf379
>> --- /dev/null
>> +++ b/board/keymile/km_arm/km_arm.c
[...]
>> +/* Multi-Purpose Pins Functionality configuration */
>> +u32 kwmpp_config[] = {
>> +     MPP0_NF_IO2,
>> +     MPP1_NF_IO3,
>> +     MPP2_NF_IO4,
>> +     MPP3_NF_IO5,
>> +     MPP4_NF_IO6,
>> +     MPP5_NF_IO7,
>> +     MPP6_SYSRST_OUTn,
>> +     MPP7_PEX_RST_OUTn,
>> +#if defined(CONFIG_KIRKWOOD_GPIO)
> 
> Why does you need this ifdef here? for a particular board this should be pre-defined.

Yep, this define should be CONFIG_SOFT_I2C.

>> +     MPP8_GPIO,      /* SDA */
>> +     MPP9_GPIO,      /* SCL */
>> +#else
>> +     MPP8_TW_SDA,
>> +     MPP9_TW_SCK,
>> +#endif
>> +     MPP10_UART0_TXD,
>> +     MPP11_UART0_RXD,
>> +     MPP12_GPO,              /* Reserved */
>> +     MPP13_UART1_TXD,
>> +     MPP14_UART1_RXD,
>> +     MPP15_GPIO,             /* Not used */
>> +     MPP16_GPIO,             /* Not used */
>> +     MPP17_GPIO,             /* Reserved */
>> +     MPP18_NF_IO0,
>> +     MPP19_NF_IO1,
>> +     MPP20_GPIO,
>> +     MPP21_GPIO,
>> +     MPP22_GPIO,
>> +     MPP23_GPIO,
>> +     MPP24_GPIO,
>> +     MPP25_GPIO,
>> +     MPP26_GPIO,
>> +     MPP27_GPIO,
>> +     MPP28_GPIO,
>> +     MPP29_GPIO,
>> +     MPP30_GPIO,
>> +     MPP31_GPIO,
>> +     MPP32_GPIO,
>> +     MPP33_GPIO,
>> +     MPP34_GPIO,             /* CDL1 (input) */
>> +     MPP35_GPIO,             /* CDL2 (input) */
>> +     MPP36_GPIO,             /* MAIN_IRQ (input) */
>> +     MPP37_GPIO,             /* BOARD_LED */
>> +     MPP38_GPIO,             /* Piggy3 LED[1] */
>> +     MPP39_GPIO,             /* Piggy3 LED[2] */
>> +     MPP40_GPIO,             /* Piggy3 LED[3] */
>> +     MPP41_GPIO,             /* Piggy3 LED[4] */
>> +     MPP42_GPIO,             /* Piggy3 LED[5] */
>> +     MPP43_GPIO,             /* Piggy3 LED[6] */
>> +     MPP44_GPIO,             /* Piggy3 LED[7] */
>> +     MPP45_GPIO,             /* Piggy3 LED[8] */
>> +     MPP46_GPIO,             /* Reserved */
>> +     MPP47_GPIO,             /* Reserved */
>> +     MPP48_GPIO,             /* Reserved */
>> +     MPP49_GPIO,             /* SW_INTOUTn */
>> +     0
>> +};
>> +
>> +int ethernet_present(void)
>> +{
>> +     uchar   buf;
>> +     int     ret = 0;
>> +
>> +#if defined(CONFIG_SUEN_P1A)
> 
> Ditto

I correct this.

> 
>> +     int     oldbusnum = i2c_get_bus_num();
>> +
>> +     i2c_set_bus_num(io_dev);
>> +     if (i2c_read(0x74, 0, 1, &buf, 1) != 0) {
>> +             printf ("%s: Error reading EEprom\n", __FUNCTION__);
>> +             i2c_set_bus_num(oldbusnum);
>> +             return -1;
>> +     }
>> +     if ((buf & 0x80) == 0x80) {
>> +             ret = 1;
>> +     }
>> +     i2c_set_bus_num(oldbusnum);
>> +#else
>> +     if (i2c_read(0x10, 2, 1, &buf, 1) != 0) {
>> +             printf ("%s: Error reading Boco\n", __FUNCTION__);
>> +             return -1;
>> +     }
>> +     if ((buf & 0x40) == 0x40) {
>> +             ret = 1;
>> +     }
>> +#endif
>> +     return ret;
>> +}
>> +
>> +int misc_init_r(void)
>> +{
>> +     I2C_MUX_DEVICE  *i2cdev;
>> +     char *str;
>> +     int mach_type;
>> +
>> +     /* add I2C Bus for I/O Expander */
>> +     i2cdev = i2c_mux_ident_muxstring((uchar *)"pca9554a:70:a");
>> +     io_dev = i2cdev->busid;
>> +     puts("Piggy:");
>> +     if (ethernet_present() == 0)
>> +             puts (" not");
>> +     puts(" present\n");
>> +
>> +     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;
>> +}
>> +
>> +int board_init(void)
>> +{
>> +     u32 tmp;
>> +
>> +     kirkwood_mpp_conf(kwmpp_config);
>> +
>> +     /*
>> +      * The FLASH_GPIO_PIN switches between using a
>> +      * NAND or a SPI FLASH. Set this pin on start
>> +      * to NAND mode.
>> +      */
>> +     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");
>> +
>> +     /*
>> +      * arch number of board
>> +      */
>> +     gd->bd->bi_arch_number = MACH_TYPE_SUEN3;
> 
> This does not match with the board you are supporting, better to use similar name

As I said above, this boards are all the same, just different
hardwarerevisions, so theys all have one MACH_TYPE_ ...

>> +
>> +     /* address of boot parameters */
>> +     gd->bd->bi_boot_params = kw_sdram_bar(0) + 0x100;
>> +
>> +#if defined(CONFIG_KIRKWOOD_GPIO)
> 
> Avoid this at least for this board patch

Don;t understand this! Why? The board uses this pins for
I2C bitbang, so I must initialize the pins ...

>> +     /* 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);
>> +#if defined(CONFIG_SYS_EEPROM_WREN)
>> +     kw_gpio_set_valid(SUEN3_ENV_WP, 38);
>> +     kw_gpio_direction_output(SUEN3_ENV_WP, 1);
>> +#endif
>> +#endif
>> +     return 0;
>> +}
[...]
>> +
>> +#if (defined(CONFIG_MACH_SUEN3) && defined(CONFIG_HARD_I2C))
> 
> Ditto..

What do you mean here?

>> +#error I2C bus resetsequence not implemented yet.
>> +#endif
>> +
>> +#if (defined(CONFIG_MACH_SUEN3) && defined(CONFIG_SOFT_I2C))
> 
> Ditto..

What do you mean here?

>> +void set_sda (int state)
>> +{
>> +     I2C_ACTIVE;
>> +     I2C_SDA(state);
>> +}
>> +
>> +void set_scl (int state)
>> +{
>> +     I2C_SCL(state);
>> +}
>> +
>> +int get_sda (void)
>> +{
>> +     I2C_TRISTATE;
>> +     return I2C_READ;
>> +}
>> +
>> +int get_scl (void)
>> +{
>> +     return (kw_gpio_get_value(SUEN3_SCL_PIN) ? 1 : 0);
>> +}
>> +#endif
> 
> Where these are used, I could not find any reference in this patch

They are used in board/keymile/common/common.c for
the I2C deblocking sequence. (This is one of the common code
used for all keymile boards)

>> +
>> +#if defined(CONFIG_SYS_EEPROM_WREN)
>> +int eeprom_write_enable (unsigned dev_addr, int state)
>> +{
>> +     kw_gpio_set_value(SUEN3_ENV_WP, !state);
>> +
>> +     return !kw_gpio_get_value(SUEN3_ENV_WP);
>> +}
>> +#endif
>> diff --git a/board/keymile/km_arm/kwbimage.cfg
>> b/board/keymile/km_arm/kwbimage.cfg
>> new file mode 100644
>> index 0000000..bfe0889
>> --- /dev/null
>> +++ b/board/keymile/km_arm/kwbimage.cfg
>> @@ -0,0 +1,59 @@
>> +#
>> +# (C) Copyright 2010
>> +# Heiko Schocher, DENX Software Engineering, hs 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
>> +#
>> +# Refer docs/README.kwimage for more details about how-to configure
>> +# and create kirkwood boot image
>> +#
>> +
>> +# Boot Media configurations
>> +BOOT_FROM    spi     # Boot from SPI flash
>> +
>> +DATA 0xFFD10000 0x01111111
>> +DATA 0xFFD10008 0x00001100
>> +DATA 0xFFD100E0 0x1B1B1B1B
>> +DATA 0xFFD20134 0xBBBBBBBB
>> +DATA 0xFFD20138 0x00BBBBBB
>> +DATA 0xFFD20154 0x00000200
>> +DATA 0xFFD2014C 0x00001C00
>> +DATA 0xFFD20148 0x00000001
>> +DATA 0xFFD01400 0x43000400
>> +DATA 0xFFD01404 0x36343000
>> +DATA 0xFFD01408 0x2302544B
>> +DATA 0xFFD0140C 0x00000032
>> +DATA 0xFFD01410 0x0000000D
>> +DATA 0xFFD01414 0x00000000
>> +DATA 0xFFD01418 0x00000000
>> +DATA 0xFFD0141C 0x00000642
>> +DATA 0xFFD01420 0x00000040
>> +DATA 0xFFD01424 0x0000F07F
>> +DATA 0xFFD01504 0x07FFFFF1
>> +DATA 0xFFD01508 0x00000000
>> +DATA 0xFFD0150C 0x00000000
>> +DATA 0xFFD01514 0x00000000
>> +DATA 0xFFD0151C 0x00000000
>> +DATA 0xFFD01494 0x00000000
>> +DATA 0xFFD01498 0x00000000
>> +DATA 0xFFD0149C 0x0000E90F
>> +DATA 0xFFD01480 0x00000001
> 
> It is better to explain each settings here about what exactly you are programming

Ok, I try it.

>> +
>> +# End of Header extension
>> +DATA 0x0 0x0
>> diff --git a/include/configs/km_arm.h b/include/configs/km_arm.h
>> new file mode 100644
>> index 0000000..091e941
>> --- /dev/null
>> +++ b/include/configs/km_arm.h
>> @@ -0,0 +1,191 @@
[...]
>> + * I2C related stuff
>> + */
>> +#undef       CONFIG_HARD_I2C         /* I2C with hardware support */
>> +#define      CONFIG_SOFT_I2C         /* I2C bit-banged       */
>> +
>> +#if defined(CONFIG_HARD_I2C)
>> +#define      CONFIG_I2C_KIRKWOOD
>> +#define      CONFIG_I2C_KW_REG_BASE          KW_TWSI_BASE
>> +#define      CONFIG_SYS_I2C_SLAVE            0x0
>> +#define      CONFIG_SYS_I2C_SPEED            100000
>> +#endif
>> +
>> +#if defined(CONFIG_SOFT_I2C)
>> +#define      CONFIG_KIRKWOOD_GPIO            /* Enable GPIO
>> Support */
>> +#ifndef __ASSEMBLY__
>> +#include <asm/arch-kirkwood/gpio.h>
>> +extern void __set_direction(unsigned pin, int high);
>> +void set_sda (int state);
>> +void set_scl (int state);
>> +int get_sda (void);
>> +int get_scl (void);
>> +#define SUEN3_SDA_PIN        8
>> +#define SUEN3_SCL_PIN        9
>> +#define SUEN3_ENV_WP 38
> 
> Not valid for this board, this patch should explain only about mgcoge2

No, they are valid, because it uses the i2c bitbang driver...
(This file is used by *all* 4 boards ...)

I know the board name mgcoge2_arm_p1a is a little bit
confusing, but this is because this was the first hardware
version of the "suen3" board, which is a part of a hardware,
where also is a powerpc piece named "mgcoge2", and so
the first version was named to mgcoge2_arm_p1a ... the
next hardware revison was named "suen3" ...

>> +
>> +#define I2C_ACTIVE   __set_direction(SUEN3_SDA_PIN, 0)
>> +#define I2C_TRISTATE __set_direction(SUEN3_SDA_PIN, 1)
>> +#define I2C_READ     (kw_gpio_get_value(SUEN3_SDA_PIN) ? 1 : 0)
>> +#define I2C_SDA(bit) kw_gpio_set_value(SUEN3_SDA_PIN, bit);
>> +#define I2C_SCL(bit) kw_gpio_set_value(SUEN3_SCL_PIN, bit);
>> +#endif
>> +
>> +#define I2C_DELAY    udelay(3)       /* 1/4 I2C clock duration */
>> +#define I2C_SOFT_DECLARATIONS
>> +
>> +#define      CONFIG_SYS_I2C_SLAVE            0x0
>> +#define      CONFIG_SYS_I2C_SPEED            100000
>> +#endif
>> +
>> +#define CONFIG_SYS_I2C_EEPROM_ADDR   0x50
>> +#define CONFIG_SYS_I2C_EEPROM_ADDR_LEN       2
>> +
>> +#if defined(CONFIG_SYS_NO_FLASH)
>> +#define CONFIG_KM_UBI_PARTITION_NAME   "ubi0"
>> +#undef       CONFIG_FLASH_CFI_MTD
>> +#undef       CONFIG_JFFS2_CMDLINE
>> +#endif
>> +
>> +#endif /* _CONFIG_KM_ARM_H */
>> diff --git a/include/configs/mgcoge2_arm_p1a.h
>> b/include/configs/mgcoge2_arm_p1a.h
>> new file mode 100644
>> index 0000000..db38915
>> --- /dev/null
>> +++ b/include/configs/mgcoge2_arm_p1a.h
>> @@ -0,0 +1,96 @@
[...]
>> +     ""
>> +
>> +#endif /* _CONFIG_SUEN3_H */
> 
> Wrong comment...

Yep, correct it.

Thanks for reviewing!

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany