> -----Original Message-----
> From: Simon Guinot [mailto:si...@sequanux.org]
> Sent: Thursday, May 12, 2011 5:17 PM
> To: Prafulla Wadaskar
> Cc: Simon Guinot; Albert ARIBAUD; u-boot@lists.denx.de; Ashish Karkare;
> Prabhanjan Sarnaik
> Subject: Re: [PATH v7 2/2] Add support for Network Space v2
>
> Hi Prafulla,
>
> On Thu, May 12, 2011 at 03:36:48AM -0700, Prafulla Wadaskar wrote:
> >
> >
> > > -----Original Message-----
> > > From: Simon Guinot [mailto:simon.gui...@sequanux.org]
> > > Sent: Thursday, May 12, 2011 3:19 PM
> > > To: Prafulla Wadaskar
> > > Cc: Albert ARIBAUD; u-boot@lists.denx.de; Simon Guinot
> > > Subject: [PATH v7 2/2] Add support for Network Space v2
> > >
> > > This patch add support for the Network Space v2 board and parents,
> based
> > > on the Marvell Kirkwood 6281 SoC. This include Network Space (Max)
> v2
> > > and Internet Space v2.
> > >
> > > Additional information is available at:
> > > http://lacie-nas.org/doku.php?id=network_space_v2
> > >
> > > Signed-off-by: Simon Guinot <sgui...@lacie.com>
> > > ---
> > > Changes for v2:
> > > - add entries to MAINTAINERS file
> > > - move boards from root Makefile to boards.cfg
> > > - move MACH_TYPE definition into netspace_v2.h
> > > - remove CONFIG_SYS_HZ redefinition
> > > - turn PHY initialization message into debug()
> > >
> > > Changes for v3: none
> > >
> > > Changes for v4:
> > > - enhance commit message: add SoC information
> > >
> > > Changes for v5: none
> > >
> > > Changes for v6:
> > > - enable device tree support
> > > - clean some "#define" in netspace_v2.h
> > > - enhance commit message: provide description URL and mention SoC
> > > family
> > > - rebase against u-boot-{arm,marvell}/master branches
> > >
> > > Changes for v7:
> > > - rebase against u-boot-marvell/master branch
> > >
> > > MAINTAINERS | 6 +
> > > board/LaCie/netspace_v2/Makefile | 49 ++++++++++
> > > board/LaCie/netspace_v2/kwbimage.cfg | 162
> > > +++++++++++++++++++++++++++++++++
> > > board/LaCie/netspace_v2/netspace_v2.c | 144
> > > +++++++++++++++++++++++++++++
> > > board/LaCie/netspace_v2/netspace_v2.h | 39 ++++++++
> > > boards.cfg | 3 +
> > > include/configs/netspace_v2.h | 153
> > > +++++++++++++++++++++++++++++++
> > > 7 files changed, 556 insertions(+), 0 deletions(-)
> > > create mode 100644 board/LaCie/netspace_v2/Makefile
> > > create mode 100644 board/LaCie/netspace_v2/kwbimage.cfg
> > > create mode 100644 board/LaCie/netspace_v2/netspace_v2.c
> > > create mode 100644 board/LaCie/netspace_v2/netspace_v2.h
> > > create mode 100644 include/configs/netspace_v2.h
> > >
...snip..
> > > +
> > > +/* Configure and initialize PHY */
> > > +void reset_phy(void)
> > > +{
> > > + mv_phy_88e1116_init("egiga0");
> > > +}
> > > +
> > > +#define NETSPACE_V2_GPIO_BUTTON 32
> >
> > How about defining this in header file ?
>
> I don't know. The GPIO button value is only useful within this file.
> Visualise both the define and the usage on the same screen is rather
> comfortable. As an alternative, I could drop the define...
>
> What is your opinion ?
Do not drop it, I will recommend to put it in
board/LaCie/netspace_v2/netspace_v2.h.
...snip...
> > > +#endif
> >
> > There should be #elif for third board and #error for #else part
>
> I have noticed this #error in the other board include files. Is it
> really needed ? This file is only included if one machine is selected.
>
No it is not really needed, choice is yours :-)
> >
> > > +
> > > +/*
> > > + * High Level Configuration Options (easy to change)
> > > + */
> > > +#define CONFIG_FEROCEON_88FR131 /* CPU Core subversion
> */
> > > +#define CONFIG_KIRKWOOD /* SOC Family Name */
> > > +#define CONFIG_KW88F6281 /* SOC Name */
> > > +#define CONFIG_SKIP_LOWLEVEL_INIT /* disable board lowlevel_init
> */
> > > +
> > > +/*
> > > + * Commands configuration
> > > + */
> > > +#define CONFIG_SYS_NO_FLASH /* Declare no flash (NOR/SPI)
> */
> > > +#include <config_cmd_default.h>
> > > +#define CONFIG_CMD_ENV
> > > +#define CONFIG_CMD_DHCP
> > > +#define CONFIG_CMD_PING
> > > +#define CONFIG_CMD_SF
> > > +#define CONFIG_CMD_I2C
> > > +#define CONFIG_CMD_IDE
> > > +#define CONFIG_CMD_USB
> > > +
> > > +/*
> > > + * Core clock definition.
> > > + */
> > > +#define CONFIG_SYS_TCLK 166000000 /* 166MHz */
> >
> > This is not needed, defined in ..arch-kirkwood/kw88f6xxx.h
>
> Actually it is. The core clock value is not standard for this boards.
> For a 6281 SoC, TCLK is defined to 200MHz and Network Space v2 TCLK is
> 166MHz.
>
> This define has been introduced with a previous patch (already merged):
> 18c4cb0f: Kirkwood: allow to override CONFIG_SYS_TCLK
>
Okay I take my words back, I was referring kw88f6162.h :-(
> >
> > > +
> > > +/*
> > > + * mv-common.h should be defined after CMD configs since it used
> them
> > > + * to enable certain macros
> > > + */
> > > +#define CONFIG_NR_DRAM_BANKS 2
> > > +#include "mv-common.h"
> > > +
> > > +/* Remove or override few declarations from mv-common.h */
> > > +#undef CONFIG_RBTREE
> > > +#undef CONFIG_ENV_SPI_MAX_HZ
> > > +#undef CONFIG_SYS_IDE_MAXBUS
> > > +#undef CONFIG_SYS_IDE_MAXDEVICE
> > > +#undef CONFIG_SYS_PROMPT
> > > +#define CONFIG_ENV_SPI_MAX_HZ 20000000 /* 20Mhz */
> > > +#define CONFIG_SYS_IDE_MAXBUS 1
> > > +#define CONFIG_SYS_IDE_MAXDEVICE 1
> > > +#define CONFIG_SYS_PROMPT "ns2> "
> > > +
> > > +/*
> > > + * Ethernet Driver configuration
> > > + */
> > > +#ifdef CONFIG_CMD_NET
> > > +#define CONFIG_MVGBE_PORTS {1, 0} /* enable port 0 only
> */
> > > +#define CONFIG_NETCONSOLE
> > > +#endif
> > > +
> > > +/*
> > > + * SATA Driver configuration
> > > + */
> > > +#ifdef CONFIG_MVSATA_IDE
> > > +#define CONFIG_SYS_ATA_IDE0_OFFSET MV_SATA_PORT0_OFFSET
> > > +/* Network Space Max v2 use 2 SATA ports */
> > > +#ifdef CONFIG_NETSPACE_MAX_V2
> > > +#define CONFIG_SYS_ATA_IDE1_OFFSET MV_SATA_PORT1_OFFSET
> > > +#endif
> > > +#endif
> > > +
> > > +/*
> > > + * Enable GPI0 support
> > > + */
> > > +#define CONFIG_KIRKWOOD_GPIO
> > > +
> > > +/*
> > > + * File systems support
> > > + */
> > > +#define CONFIG_CMD_EXT2
> > > +#define CONFIG_CMD_FAT
> >
> > Get rid of this, use CONFIG_SYS_MVFS
>
> Outch. Please no :)
>
> CONFIG_SYS_MVFS enable support for mtd, jffs2 and ubifs.
> It is completely useless for a Network Space v2 board where the SPI
> flash size is 512KB. The whole space is for U-Boot.
Okay..
>
> >
> > > +
> > > +/*
> > > + * Use the HUSH parser
> > > + */
> > > +#define CONFIG_SYS_HUSH_PARSER
> > > +#define CONFIG_SYS_PROMPT_HUSH_PS2 "> "
> > > +
> > > +/*
> > > + * Console configuration
> > > + */
> > > +#define CONFIG_CONSOLE_MUX
> > > +#define CONFIG_SYS_CONSOLE_IS_IN_ENV
> > > +
> > > +/*
> > > + * Enable device tree support
> > > + */
> > > +#define CONFIG_OF_LIBFDT
> > > +
> > > +/*
> > > + * Environment variables configurations
> > > + */
> > > +#define CONFIG_ENV_IS_IN_SPI_FLASH
> > > +#define CONFIG_ENV_SECT_SIZE 0x10000 /* 64KB */
> > > +#define CONFIG_ENV_SIZE 0x1000 /* 4KB */
> > > +#define CONFIG_ENV_ADDR 0x70000
> > > +#define CONFIG_ENV_OFFSET 0x70000 /* env starts here */
> > > +
> > > +/*
> > > + * Default environment variables
> > > + */
> > > +#define CONFIG_BOOTARGS "console=ttyS0,115200"
> > > +
> > > +#define CONFIG_BOOTCOMMAND \
> > > + "if run usbload || run diskload; then bootm 0x800000; fi"
> > > +
> > > +#define CONFIG_EXTRA_ENV_SETTINGS \
> > > + "stdin=serial,nc\0" \
> > > + "stdout=serial,nc\0" \
> > > + "stderr=serial,nc\0" \
> > > + "ipaddr=192.168.1.111\0" \
> >
> > NAK for ipaddr, no ip address should be defined by default.
>
> I understand, but I need a known IP address.
>
> I want people to be able to update the stock U-Boot using netconsole
> (without a serial link which require to open the case). At restart,
> right after the update, a user must be able to figure out the board IP.
>
> An alternative could be using a DHCP configuration.
I wish to know Wolfgang's or Ben's comments on this.
Because I remember strategically hard coding ipaddress is not recommend.
Regards..
Prafulla . .
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
