Dear Prafulla Wadaskar,

In message <1238798370-9245-5-git-send-email-prafu...@marvell.com> you wrote:
> From: prafulla_wadaskar <prafu...@marvell.com>
> 
> This is Marvell's 88F6281_A0 based custom board developed
> for wireless access point product
> 
> This patch is tested for-
> 1. Boot from DRAM/SPI flash/NFS
> 2. File transfer using tftp and loadb
> 3. SPI flash read/write/erase
> 4. Booting Linux kernel and RFS from SPI flash
> 
> Signed-off-by: prafulla_wadaskar <prafu...@marvell.com>
> Reviewed by: Ronen Shitrit <rshit...@marvell.com>
> Tested by: Piyush Shah <spiy...@marvell.com>
...
> --- a/MAKEALL
> +++ b/MAKEALL
> @@ -506,6 +506,7 @@ LIST_ARM9="                       \
>       lpd7a400                \
>       mx1ads                  \
>       mx1fs2                  \
> +     mv88f6281gtw_ge         \
>       netstar                 \
>       nmdk8815                \
>       omap1510inn             \

Please keep lists sorted.

> diff --git a/Makefile b/Makefile
> index 8bf36ce..eb2fdfb 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2564,6 +2564,9 @@ DB64360_config: unconfig
>  DB64460_config:      unconfig
>       @$(MKCONFIG) DB64460 ppc 74xx_7xx db64460 Marvell
>  
> +mv88f6281gtw_ge_config: unconfig
> +     @$(MKCONFIG) $(@:_config=) arm arm926ejs $(@:_config=) Marvell kirkwood
> +
>  ELPPC_config: unconfig
>       @$(MKCONFIG) $(@:_config=) ppc 74xx_7xx elppc eltec

Please keep lists sorted.

> diff --git a/board/Marvell/mv88f6281gtw_ge/bin_dep.sh 
> b/board/Marvell/mv88f6281gtw_ge/bin_dep.sh
> new file mode 100755
> index 0000000..5cb1e36
> --- /dev/null
> +++ b/board/Marvell/mv88f6281gtw_ge/bin_dep.sh
> @@ -0,0 +1,74 @@
...
> +CUR_DIR=$2
> +TOP_DIR=`pwd`
> +BIN_FILE=$TOP_DIR/$1
> +
> +DOIMAGE=$TOP_DIR/cpu/arm926ejs/kirkwood/doimage/doimage


Please explain why this would be needed for?

Note that this most probably will not fork with out of tree builds.

...
> diff --git a/board/Marvell/mv88f6281gtw_ge/dramregs_333h.txt 
> b/board/Marvell/mv88f6281gtw_ge/dramregs_333h.txt
> new file mode 100644
> index 0000000..d40a9c7
> --- /dev/null
> +++ b/board/Marvell/mv88f6281gtw_ge/dramregs_333h.txt
> @@ -0,0 +1,27 @@
> +0xFFD01400 0x43000a00
> +0xFFD01404 0x38543000
> +0xFFD01408 0x2202433D
> +0xFFD0140C 0x0000002A
> +0xFFD01410 0x0000000D
> +0xFFD01414 0x00000000
> +0xFFD01418 0x00000000
> +0xFFD0141C 0x00000c52
> +0xFFD01420 0x00000046
> +0xFFD01424 0x0000F1FF
> +0xFFD01428 0x00085520
> +0xFFD0147c 0x00008552
> +0xFFD01508 0x00000000
> +0xFFD01504 0x07FFFFF1
> +0xFFD0150C 0x00000000
> +0xFFD01514 0x00000000
> +0xFFD0151C 0x00000000
> +0xFFD01494 0x00010001
> +0xFFD01498 0x00000000
> +0xFFD0149C 0x0000E811
> +0xFFD01480 0x00000001
> +0xFFD20204 0x00000000
> +0xFFD100E0 0x1B1B1B9B
> +0xFFD100D8 0x00000060
> +0xFFD1007C 0x00000068
> +0xFFD50430 0x00000008
> +0x0 0x0

What sort of file is this? Under which license is it?

> diff --git a/board/Marvell/mv88f6281gtw_ge/mv88f6281gtw_ge.c 
> b/board/Marvell/mv88f6281gtw_ge/mv88f6281gtw_ge.c
> new file mode 100644
> index 0000000..b88fe3e
> --- /dev/null
> +++ b/board/Marvell/mv88f6281gtw_ge/mv88f6281gtw_ge.c
> @@ -0,0 +1,135 @@
...
> +#ifndef MV88F6281GTW_GE_DEBUG
> +#define MV88F6281GTW_GE_DEBUG                0
> +#endif
> +#define DEBUG_PRINT          MV88F6281GTW_GE_DEBUG

Unused code? Delete!

> +#include <common.h>
> +#include <debug_prints.h>

Non existent file included here.

> +/* this line must be removed after this machine name gets mainlined in 
> mach_types.h*/
> +#define MACH_TYPE_MV88F6281GTW_GE      1932

Please first register the MACH ID, then fix this, then resubmit.


> +DECLARE_GLOBAL_DATA_PTR;
> +
> +#define MV88F6281GTW_GE_OE_VAL_LOW   (BIT20) /*make GLED on */
> +#define MV88F6281GTW_GE_OE_VAL_HIGH  ((BIT6)|(BIT13)|(BIT16)|(BIT17))
> +#define MV88F6281GTW_GE_OE_LOW               (~((BIT7) | (BIT20) | (BIT21))) 
> /*enable GLED,RLED */
> +#define MV88F6281GTW_GE_OE_HIGH              
> (~((BIT4)|(BIT6)|(BIT7)|(BIT12)|(BIT13)|(BIT16)|(BIT17)))

Lines too long (also check in all other files).

> +/*
> + * Default values for MPP registers
> + */
> +#define MV88F6281GTW_GE_MPP0_7               0x01112222
> +#define MV88F6281GTW_GE_MPP8_15              0x11103311
> +#define MV88F6281GTW_GE_MPP16_23     0x00001111
> +#define MV88F6281GTW_GE_MPP24_31     0x22222222
> +#define MV88F6281GTW_GE_MPP32_39     0x40440222
> +#define MV88F6281GTW_GE_MPP40_47     0x00004444
> +#define MV88F6281GTW_GE_MPP48_55     0x00000000
> +
> +/*
> + * function definitations
> + */
> +#ifdef CONFIG_SWITCH_88E61XX
> +extern int mv_switch_88e61xx_init(u32 eth_port_num);
> +#endif

Do not use extern. Privde proper prototypes in the respective header
files.

> +int board_init(void)
> +{
...
> +     /* relocate the exception vectors */
> +     /* U-Boot is running from DRAM at this stage */
> +     for (i = 0; i < 0x100; i += 4) {
> +             *(unsigned int *)(0x0 + i) = *(unsigned int *)(TEXT_BASE + i);
> +     }

Are you absolutely sure this code is correct?

> +int dram_init(void)
> +{
> +     int i;
> +
> +     debug_print_ftrace();

You probably want to get rid of these. I don't think we will accept
this.

> diff --git a/board/Marvell/mv88f6281gtw_ge/u-boot.lds 
> b/board/Marvell/mv88f6281gtw_ge/u-boot.lds
> new file mode 100644
> index 0000000..0338757
> --- /dev/null
> +++ b/board/Marvell/mv88f6281gtw_ge/u-boot.lds
> @@ -0,0 +1,53 @@
> +/*
> + * (C) Copyright 2009
> + * Marvell Semiconductor <www.marvell.com>
> + * Prafulla Wadaskar <prafu...@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
> + */
> +
> +OUTPUT_FORMAT("elf32-littlearm", "elf32-littlearm", "elf32-littlearm")
> +OUTPUT_ARCH(arm)
> +ENTRY(_start)
> +SECTIONS
> +{
> +     . = _start;
> +     . = ALIGN(4);
> +     .text   :
> +     {
> +       cpu/arm926ejs/start.o (.text)
> +       *(.text)
> +     }
> +     .rodata : { *(.rodata) }
> +     . = ALIGN(4);
> +     .data : { *(.data) }
> +     . = ALIGN(4);
> +     .got : { *(.got) }
> +
> +     . = .;
> +     __u_boot_cmd_start = .;
> +     .u_boot_cmd : { *(.u_boot_cmd) }
> +     __u_boot_cmd_end = .;
> +
> +     . = ALIGN(4);
> +     __bss_start = .;
> +     .bss (NOLOAD) : { *(.bss) . = ALIGN(4); }
> +     _end = .;
> +}
> +
> diff --git a/include/configs/mv88f6281gtw_ge.h 
> b/include/configs/mv88f6281gtw_ge.h
> new file mode 100644
> index 0000000..70e6dca
> --- /dev/null
> +++ b/include/configs/mv88f6281gtw_ge.h
...
> +/*
> + * Above definitions used in below file, do not change the sequence
> + */
> +#include <configs/kirkwood.h>        /* CPU specific information */

include/configs/ is strictly for board specific information only; CPU
specific data has no place there.

...
> +#define      CONFIG_BOOTMAPSZ                (8<<20) /* Initial Memmap for 
> Linux */
> +#define CONFIG_CMDLINE_TAG   1       /* enable passing of ATAGs  */
> +#define CONFIG_INITRD_TAG    1       /* enable INITRD tag */
> +#define CONFIG_SETUP_MEMORY_TAGS 1   /* enable memory tag */
> +
> +#define      CONFIG_SYS_PROMPT               "Marvell>> "    /* Command 
> Prompt */
> +#define      CONFIG_SYS_CBSIZE               1024    /* Console I/O Buff 
> Size */
> +#define      CONFIG_SYS_PBSIZE               (CONFIG_SYS_CBSIZE \
> +                             +sizeof(CONFIG_SYS_PROMPT)+16)  /* Print Buff */

Lines too long, here and elsewhere. Please fix globally.

> + * Commands configuration
> + */
> +#define CONFIG_CMD_ENV
> +#define CONFIG_CMD_RUN
> +#define CONFIG_CMD_LOADB
> +#define CONFIG_CMD_NET
> +#define CONFIG_CMD_DHCP
> +#define CONFIG_CMD_AUTOSCRIPT

AUTOSCRIPT is deprecated. Please use SOURCE instead.


> +#ifdef CONFIG_SPI_FLASH
> +#define CONFIG_ENV_IS_IN_SPI_FLASH   1
> +#define CONFIG_ENV_SIZE                      _64K    /* 1 spi flash block */
> +#define CONFIG_ENV_SECT_SIZE         _64K

Please use real numbers  and get rid of such "cool" constants.


> +/* Default environment variables */
> +#define CONFIG_BOOTCOMMAND   "$(x_bootcmd_kernel); setenv bootargs 
> $(x_bootargs) $(x_bootargs_root); bootm 0x6400000; "
> +#define CONFIG_EXTRA_ENV_SETTINGS \
> +     "x_bootargs=console=ttyS0,115200 
> mtdparts=spi0.0:512k(uboot),5...@512k(psm),2...@1m(kernel),1...@3m(rootfs)\0" 
> \


Lines WAY too long.

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
Sometimes a feeling is all we humans have to go on.
        -- Kirk, "A Taste of Armageddon", stardate 3193.9
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to