Dear Siddarth Gore,
In message <1268660568-23022-1-git-send-email-go...@marvell.com> you wrote:
...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 80057ce..f102cd8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -765,6 +765,10 @@ Prafulla Wadaskar <prafu...@marvell.com>
> rd6281a ARM926EJS (Kirkwood SoC)
> sheevaplug ARM926EJS (Kirkwood SoC)
>
> +Siddarth Gore <go...@marvell.com>
> +
> + guruplug ARM926EJS (Kirkwood SoC)
> +
> Richard Woodruff <r-woodru...@ti.com>
Please keep list of maintainers sorted by last name.
> --- /dev/null
> +++ b/board/Marvell/guruplug/config.mk
> @@ -0,0 +1,28 @@
...
> +TEXT_BASE = 0x00600000
> +
> +# Kirkwood Boot Image configuration file
Please don't add such trailers. Move the description upto the start of
the file - if you really think you need one. It's probably better just
to drop this.
> +KWD_CONFIG = $(SRCTREE)/board/$(BOARDDIR)/kwbimage.cfg
> diff --git a/board/Marvell/guruplug/guruplug.c
> b/board/Marvell/guruplug/guruplug.c
> new file mode 100644
> index 0000000..ba47ca1
> --- /dev/null
> +++ b/board/Marvell/guruplug/guruplug.c
> @@ -0,0 +1,167 @@
> +/*
> + * (C) Copyright 2009
> + * Marvell Semiconductor <www.marvell.com>
> + * Written-by: Siddarth Gore <go...@marvell.com>
> + *
> + * 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
> + */
> +
> +#include <common.h>
> +#include <miiphy.h>
> +#include <asm/arch/kirkwood.h>
> +#include <asm/arch/mpp.h>
> +#include "guruplug.h"
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +int board_init(void)
> +{
> + /*
> + * default gpio configuration
> + * There are maximum 64 gpios controlled through 2 sets of registers
> + * the below configuration configures mainly initial LED status
> + */
> + kw_config_gpio(GURUPLUG_OE_VAL_LOW,
> + GURUPLUG_OE_VAL_HIGH,
> + GURUPLUG_OE_LOW, GURUPLUG_OE_HIGH);
> +
> + /* 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_GPO, /* GPIO_RST */
> + MPP8_TW_SDA,
> + MPP9_TW_SCK,
> + MPP10_UART0_TXD,
> + MPP11_UART0_RXD,
> + MPP12_SD_CLK,
> + MPP13_SD_CMD,
> + MPP14_SD_D0,
> + MPP15_SD_D1,
> + MPP16_SD_D2,
> + MPP17_SD_D3,
> + MPP18_NF_IO0,
> + MPP19_NF_IO1,
> + MPP20_GE1_0,
> + MPP21_GE1_1,
> + MPP22_GE1_2,
> + MPP23_GE1_3,
> + MPP24_GE1_4,
> + MPP25_GE1_5,
> + MPP26_GE1_6,
> + MPP27_GE1_7,
> + MPP28_GE1_8,
> + MPP29_GE1_9,
> + MPP30_GE1_10,
> + MPP31_GE1_11,
> + MPP32_GE1_12,
> + MPP33_GE1_13,
> + MPP34_GE1_14,
> + MPP35_GE1_15,
> + MPP36_GPIO,
> + MPP37_GPIO,
> + MPP38_GPIO,
> + MPP39_GPIO,
> + MPP40_TDM_SPI_SCK,
> + MPP41_TDM_SPI_MISO,
> + MPP42_TDM_SPI_MOSI,
> + MPP43_GPIO,
> + MPP44_GPIO,
> + MPP45_GPIO,
> + MPP46_GPIO, /* M_RLED */
> + MPP47_GPIO, /* M_GLED */
> + MPP48_GPIO, /* B_RLED */
> + MPP49_GPIO, /* B_GLED */
> + 0
> + };
> + kirkwood_mpp_conf(kwmpp_config);
> +
> + /*
> + * arch number of board
> + */
> + gd->bd->bi_arch_number = MACH_TYPE_GURUPLUG;
> +
> + /* adress of boot parameters */
> + gd->bd->bi_boot_params = kw_sdram_bar(0) + 0x100;
> +
> + return 0;
> +}
> +
> +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 = kw_sdram_bs(i);
> + }
> + return 0;
> +}
> +
> +#ifdef CONFIG_RESET_PHY_R
> +void mv_phy_88e1121_init(char *name)
> +{
> + u16 reg;
> + u16 devadr;
> +
> + if (miiphy_set_current_dev(name))
> + return;
> +
> + /* command to read PHY dev address */
> + if (miiphy_read(name, 0xEE, 0xEE, (u16 *) &devadr)) {
> + printf("Err..%s could not read PHY dev address\n",
> + __FUNCTION__);
> + return;
> + }
> +
> + /*
> + * Enable RGMII delay on Tx and Rx for CPU port
> + * Ref: sec 4.7.2 of chip datasheet
> + */
> + miiphy_write(name, devadr, MV88E1121_PGADR_REG, 2);
> + miiphy_read(name, devadr, MV88E1121_MAC_CTRL2_REG, ®);
> + reg |= (MV88E1121_RGMII_RXTM_CTRL | MV88E1121_RGMII_TXTM_CTRL);
> + miiphy_write(name, devadr, MV88E1121_MAC_CTRL2_REG, reg);
> + miiphy_write(name, devadr, MV88E1121_PGADR_REG, 0);
> +
> + /* reset the phy */
> + if (miiphy_read (name, devadr, PHY_BMCR, ®) != 0) {
> + printf("Err..(%s) PHY status read failed\n", __FUNCTION__);
> + return;
> + }
> + if (miiphy_write (name, devadr, PHY_BMCR, reg | 0x8000) != 0) {
> + printf("Err..(%s) PHY reset failed\n", __FUNCTION__);
> + return;
> + }
> +
> + printf("88E1121 Initialized on %s\n", name);
> +}
We have pretty much identical code already in mv_phy_88e1116_init()
[in "board/Marvell/rd6281a/rd6281a.c"], in reset_phy() [in
"board/Marvell/openrd_base/openrd_base.c"], in reset_phy(0 [in
"board/Marvell/sheevaplug/sheevaplug.c"] and I don't know where else.
I object against adding more and more copies of the same code. Please
factor out the common part into a single implementation, and call this
everwhere where such code is used. Thanks.
...
> --- /dev/null
> +++ b/board/Marvell/guruplug/guruplug.h
> @@ -0,0 +1,39 @@
....
> +#define GURUPLUG_OE_LOW (~(0))
> +#define GURUPLUG_OE_HIGH (~(0))
Is this correct? Both LOW and HIGH use the same value??
...
> +#define CONFIG_SYS_NO_FLASH /* Declare no flash (NOR/SPI) */
> +#include <config_cmd_default.h>
> +#define CONFIG_CMD_AUTOSCRIPT
CONFIG_CMD_AUTOSCRIPT has been deprecated long ago. Please fix.
> +/*
> + * Other required minimal configurations
> + */
In which way is this "minimal" ?
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: w...@denx.de
Uncertain fortune is thoroughly mastered by the equity of the calcu-
lation. - Blaise Pascal
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
