[U-Boot] [PATCH] Marvell GuruPlug Board Support

Siddarth Gore gores at marvell.com
Thu Mar 18 06:28:45 CET 2010


On Mon, 2010-03-15 at 09:08 -0700, Wolfgang Denk wrote:
> Dear Siddarth Gore,
> 
> In message <1268660568-23022-1-git-send-email-gores at marvell.com> you wrote:
> ...
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 80057ce..f102cd8 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -765,6 +765,10 @@ Prafulla Wadaskar <prafulla at marvell.com>
> >       rd6281a         ARM926EJS (Kirkwood SoC)
> >       sheevaplug      ARM926EJS (Kirkwood SoC)
> >
> > +Siddarth Gore <gores at marvell.com>
> > +
> > +     guruplug        ARM926EJS (Kirkwood SoC)
> > +
> >  Richard Woodruff <r-woodruff2 at ti.com>
> 
> Please keep list of maintainers sorted by last name.
> 
ok. done.

> > --- /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.
> 
ok. removed this comment.

> > +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 <gores at 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);
> > +     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, &reg) != 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.
> 
i can create a new file ethphy.c in board/Marvell/common and put this
code there. but if we are going to move to a phy driver (as Prafulla
suggested) then that wont be necessary. Please advise.

Also i think we should maintain different init functions for 1116 and
1121 as they represent different phy families, even though the code is
pretty much the same right now.

> 
> ...
> > --- /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.
> 
ok. removed this.

> > +/*
> > + * Other required minimal configurations
> > + */
> 
> In which way is this "minimal" ?
> 
ok. removed the word 'minimal' from the comment.

-siddarth

> 
> 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
> Uncertain fortune is thoroughly mastered by the equity of the  calcu-
> lation.                                               - Blaise Pascal



More information about the U-Boot mailing list