[U-Boot] [PATCH] Marvell Kirkwood family SOC support
Wolfgang Denk
wd at denx.de
Fri Apr 3 21:15:35 CEST 2009
Dear Prafulla Wadaskar,
In message <1238798370-9245-2-git-send-email-prafulla at marvell.com> you wrote:
> From: prafulla_wadaskar <prafulla at marvell.com>
>
> Kirkwood family controllers are highly integrated SOCs
> based on Feroceon-88FR131/Sheeva-88SV131 cpu core.
>
> SOC versions supported:-
> 1) 88F6281-Z0 define CONFIG_KW88F6281_Z0
> 2) 88F6281-A0 define CONFIG_KW88F6281_A0
> 3) 88F6192-A0 define CONFIG_KW88F6192_A0
>
> Other supported features:-
> 1) Doimage utility needed to create binaries with
> bootROM header
> 2) get_random_hex() fucntion
> 3) SPI port controller driver
> 4) PCI Express port initialization
>
> Contributors:
> Yotam Admon <yotam at marvell.com>
> Michael Blostein <michaelbl at marvell.com
>
> Signed-off-by: prafulla_wadaskar <prafulla at marvell.com>
> Reviewed by: Ronen Shitrit <rshitrit at marvell.com>
...
> board/Marvell/common/kw_lowlevel_init.S | 45 +
> board/Marvell/include/core.h | 4 +
> cpu/arm926ejs/kirkwood/Makefile | 52 +
> cpu/arm926ejs/kirkwood/bin_dep.sh | 50 +
> cpu/arm926ejs/kirkwood/config.mk | 25 +
> cpu/arm926ejs/kirkwood/doimage/Makefile | 112 ++
> cpu/arm926ejs/kirkwood/doimage/bootstrap_def.h | 88 ++
> cpu/arm926ejs/kirkwood/doimage/doimage | Bin 0 -> 17712 bytes
Never ever add binaries !!!
> --- a/board/Marvell/include/core.h
> +++ b/board/Marvell/include/core.h
> @@ -12,9 +12,11 @@ space). The macros take care of Big/Little endian conversions.
> #ifndef __INCcoreh
> #define __INCcoreh
>
> +#ifndef CONFIG_KIRKWOOD
> #include "mv_gen_reg.h"
>
> extern unsigned int INTERNAL_REG_BASE_ADDR;
> +#endif /* CONFIG_KIRKWOOD */
>
> /****************************************/
> /* GENERAL Definitions */
> @@ -91,10 +93,12 @@ extern unsigned int INTERNAL_REG_BASE_ADDR;
> #define _1G 0x40000000
> #define _2G 0x80000000
Please get rid of these "cool" (actually stupid) macros.
> +#ifndef __ASSEMBLY__
> #ifndef BOOL_WAS_DEFINED
> #define BOOL_WAS_DEFINED
> typedef enum _bool{false,true} bool;
> #endif
> +#endif
>
> /* Little to Big endian conversion macros */
Please also use standard macros here, and clean up these proprietary
definitions.
> diff --git a/cpu/arm926ejs/kirkwood/bin_dep.sh b/cpu/arm926ejs/kirkwood/bin_dep.sh
> new file mode 100755
> index 0000000..1b36e91
> --- /dev/null
> +++ b/cpu/arm926ejs/kirkwood/bin_dep.sh
...
> +TOP_DIR=`pwd`
> +CUR_DIR=$2
> +BIN_FILE=$TOP_DIR/$1
> +
> +if [ "clean" == "$1" ]; then
> + # erase created doimage utility
> + rm -f $CUR_DIR/doimage/doimage
> + exit 0
> +fi
> +
> +if [ ! -f $BIN_FILE ]; then
> + echo Error.. could not find $BIN_FILE
> + exit 1
> +else
> + make -C $CUR_DIR/doimage
> + exit 0
> +fi
Please get rid of this. It doesn't for for out-of-tree builds anyway.
...
> diff --git a/cpu/arm926ejs/kirkwood/doimage/Makefile b/cpu/arm926ejs/kirkwood/doimage/Makefile
> new file mode 100644
> index 0000000..219ae1e
> --- /dev/null
> +++ b/cpu/arm926ejs/kirkwood/doimage/Makefile
> @@ -0,0 +1,112 @@
..
> +BIN_FILES = doimage$(SFX)
> +
> +OBJ_LINKS =
> +OBJ_FILES = doimage.o
> +
> +LIBFDT_OBJ_FILES =
> +
> +#-------------------------------------------------------------------------
> +
> +HOSTARCH := $(shell uname -m | \
> + sed -e s/i.86/i386/ \
> + -e s/sun4u/sparc64/ \
> + -e s/arm.*/arm/ \
> + -e s/sa110/arm/ \
> + -e s/powerpc/ppc/ \
> + -e s/Power\ Macintosh/ppc/ \
> + -e s/macppc/ppc/)
> +
> +HOSTOS := $(shell uname -s | tr A-Z a-z | \
> + sed -e 's/\(cygwin\).*/cygwin/')
Why is this needed? You should get this from the top level Makefile.
> +#########################################################################
> diff --git a/cpu/arm926ejs/kirkwood/doimage/bootstrap_def.h b/cpu/arm926ejs/kirkwood/doimage/bootstrap_def.h
> new file mode 100644
> index 0000000..8782206
> --- /dev/null
> +++ b/cpu/arm926ejs/kirkwood/doimage/bootstrap_def.h
Do you REEALLY need all this stuff?
Why? What is the rationale behind it?
Did I miss the documentation for it?
Apropos documentation... how about entries to the MAINTAINERS file,
MAKEALL, etc. ?
...
> +/* typedefs */
> +
> +typedef char s8;
> +typedef unsigned char u8;
> +
> +typedef int s32;
> +typedef unsigned int u32;
> +
> +typedef short s16;
> +typedef unsigned short u16;
> +
> +typedef long s64;
> +typedef unsigned long u64;
> +
Why is theis needed? Please include the respexctive standard headers.
> diff --git a/cpu/arm926ejs/kirkwood/doimage/doimage b/cpu/arm926ejs/kirkwood/doimage/doimage
> new file mode 100755
> index 0000000000000000000000000000000000000000..9fee11740454ff880c081a64be0d692b1da4c19a
> GIT binary patch
> literal 17712
> zcmcIse|%KcmA{h=Fd#ayQbf(l!zLA42nnP#P^kP$gccORAQmtT$%Gl3WMbxx1Q%-J
> zWR$m$X;~#*yJ8DU`*B;_;#LAEQxjlGw01XGYjJg3RMZ)RHtSDnquKBG-uEVNNWk`w
> zeSB{2Irp4<?m6e4d+vR2-s8^FC1oy`OW0SI$QH!5-{bZaqwb!oD8)h(9x+kmiYvr$
...
Never, never ever do this again.
> diff --git a/cpu/arm926ejs/kirkwood/doimage/doimage.c b/cpu/arm926ejs/kirkwood/doimage/doimage.c
> new file mode 100644
> index 0000000..0c484b2
> --- /dev/null
> +++ b/cpu/arm926ejs/kirkwood/doimage/doimage.c
> @@ -0,0 +1,1341 @@
Why is this needed?
...
> +#define DOIMAGE_VERSION "1.00"
> +void print_usage(void);
> +int f_in = -1;
> +int f_out = -1;
> +int f_header = -1;
> +typedef enum { IMG_SATA, IMG_UART, IMG_FLASH, IMG_BOOTROM, IMG_NAND, IMG_HEX,
> + IMG_BIN, IMG_PEX, IMG_I2C
> +} IMG_TYPE;
> +typedef enum { HDR_IMG_ONE_FILE = 1, /* Create one file with header and image */
> + HDR_IMG_TWO_FILES = 2, /* Create seperate header and image files */
> + HDR_ONLY = 3, /* Create only header */
> + IMG_ONLY = 4 /* Create only image */
> +} HEADER_MODE;
Why do you have to reinvent your own image format here?
U-Boot already has a prtty powerful and flexible format - uns this.
[Keep in mind that one day you want to add this to Linux, too. RMK
will be much more draconic about such stuff than me.]
> +/* 8 bit checksum */
> +u8 checksum8(void *start, u32 len, u8 csum)
...
> +/* 32 bit checksum */
> +u32 checksum32(void *start, u32 len, u32 csum)
...
> +void make_crc_table(u32 * crc_table)
No, no, no. You really re-invent all parts of the wheel.
Stop here. Don't. NAK.
...
> +void print_usage(void)
> +{
> + printf("\n");
> + printf("Marvell doimage Tool version %s\n", DOIMAGE_VERSION);
> + printf("Supported SoC devices: \n");
> + printf(" Marvell 88F6082 - A1\n");
> + printf(" Marvell 88F6180 - A0\n");
> + printf(" Marvell 88F6192 - A0\n");
> + printf(" Marvell 88F6190 - A0\n");
> + printf(" Marvell 88F6281 - A0\n");
> + printf("\n");
> + printf("usage: \n");
> + printf
> + ("doimage <must_options> [other_options] image_in image_out [header_out]\n");
> + printf("\n<must_options> - can be one or more of the following:\n\n");
> + printf("-T image_type: sata\\uart\\flash\\bootrom\\nand\\hex\\pex\n");
> + printf(" if image_type is sata, the image_out will\n");
> + printf(" include header only.\n");
> + printf("-D image_dest: image destination in dram (in hex)\n");
> + printf("-E image_exec: execution address in dram (in hex)\n");
> + printf
> + (" if image_type is 'flash' and image_dest is 0xffffffff\n");
> + printf(" then execution address on the flash\n");
> + printf
> + ("-S image_source: if image_type is sata then the starting sector of\n");
> + printf
> + (" the source image on the disk - mandatory for sata\n");
> + printf
> + (" if image_type is flash\\nand then the starting offset of\n");
> + printf
> + (" the source image at the flash - optional for flash\\nand\n");
> + printf("-W hex_width : HEX file width, can be 8,16,32,64 \n");
> + printf
> + ("-M twsi_file: ascii file name that contains the I2C init regs set by h/w.\n");
> + printf(" this is used in i2c boot only\n");
> + printf
> + ("\n<other_options> - optional and can be one or more of the following:\n\n");
> + printf
> + ("-R dram_file: ascii file name that contains the list of dram regs\n");
> + printf("-P nand_page_size (decimal 512, 2048, ..): NAND Page size\n");
> + printf("-C nand_ecc_mode (1=Hamming, 2=RS, 3=None)\n");
> + printf("-L delay in mili seconds before DRAM init\n");
> + printf("-I copy image in PIO mode (valid for SATA only)\n");
> + printf("-X pre_padding_size (hex)\n");
> + printf("-Y post_padding_size (hex)\n");
> + printf("-H header_mode: Header mode, can be:\n");
> + printf
> + (" -H 1 :will create one file (image_out) for header and image\n");
> + printf
> + (" -H 2 :will create two files, (image_out) for image , (header_out) for header\n");
> + printf(" -H 3 :will create one file (image_out) for header only \n");
> + printf(" -H 4 :will create one file (image_out) for image only \n");
> + printf("\ncommand possibilities: \n\n");
> + printf("doimage -T hex -W width image_in image_out\n");
> + printf("doimage -T bootrom image_in image_out\n");
> + printf("doimage -T sata -S sector -D image_dest -E image_exec\n");
> + printf(" [other_options] image_in image_out header_out\n\n");
> + printf("doimage -T flash -D image_dest -E image_exec [-S address]\n");
> + printf(" [other_options] image_in image_out\n\n");
> + printf("doimage -T pex -D image_dest -E image_exec \n");
> + printf(" [other_options] image_in image_out\n\n");
> + printf
> + ("doimage -T nand -D image_dest -E image_exec [-S address] -P page_size\n");
> + printf(" [other_options] image_in image_out\n\n");
> + printf("doimage -T uart -D image_dest -E image_exec\n");
> + printf(" [other_options] image_in image_out\n\n");
> + printf("doimage -T pex -D image_dest -E image_exec \n");
> + printf(" [other_options] image_in image_out\n\n");
> + printf
> + ("doimage -T i2c -D image_dest -E image_exec -M twsi_init_file\n");
> + printf(" [other_options] image_in image_out\n\n");
> + printf("\n\n\n");
Instead of splitting this into a zillion or printf() statements (where
puts() would be sifficient, too), just feed the text into one
statement.
But anyway - I think you need to re-think this concept.
Such code has zero chance to go into mainline - not this image
building code, and not the U-Boot implementation that obviusly
requires that.
> diff --git a/cpu/arm926ejs/kirkwood/dram.c b/cpu/arm926ejs/kirkwood/dram.c
> new file mode 100644
> index 0000000..41f1316
> --- /dev/null
> +++ b/cpu/arm926ejs/kirkwood/dram.c
...
> +u32 kw_get_dram_bank_base_addr(MEMORY_BANK bank)
> +{
> + u32 result = 0;
> + u32 enable = (0x01 & KW_REG_READ((0x1504 + bank * 8)));
> +
> + if ((!enable) || (bank > BANK3))
> + return 0;
> +
> + result = KW_REG_READ((0x1500 + bank * 8));
Here and everywhere: do not access any device registers based on
address plus offset. Provide proper C structures instead, and access
the struct elements so the compiler can perform appropriate type
checking.
And please do not re-invent your own private accessor macros /
functions.
> +u32 kw_get_dram_bank_size(MEMORY_BANK bank)
> +{
> + u32 result = 0;
> + u32 enable = (0x01 & KW_REG_READ((0x1504 + bank * 8)));
> +
> + if ((!enable) || (bank > BANK3))
> + return 0;
> + result = (0xff000000 & KW_REG_READ((0x1504 + bank * 8)));
> + result += 0x01000000;
> + return result;
Consider to auto-size the RAM ionstead of using static information
from some registers (which may or may not be correct).
> +/*
> + * Generates Ramdom hex number reading some time varient system registers
> + * and using md5 algorithm
> + */
> +unsigned char get_random_hex(void)
> +{
You probably want to make this optional, so it can be omitted. Always
requesting the full md5 code just to get some random numbers is a lot
of overhead.
BTW - what do you need random numbers for in U-Boot? Nobody else needs
this...
...
> + return KW_OK;
Here and everywhere: please write readable code and avoid custom
macros like these.
...
> + debug_print_ftrace();
> + /*CPU streaming & write allocate */
> + env = getenv("enaWrAllo");
Documentation for all these variables missing.
Please use readable / writable variable names. CamelCase is strongly
deprecated.
> + /* Verify write allocate and streaming */
> + printf("\n");
> + __asm__ __volatile__("mrc p15, 1, %0, c15, c1, 0":"=r"(temp));
> + if (temp & BIT29)
> + info_print("Streaming enabled");
> + else
> + info_print("Streaming disabled");
> + if (temp & BIT28)
> + info_print("Write allocate enabled");
> + else
> + info_print("Write allocate disabled");
Yet another set of proprietary functions. Get rid of these!
> diff --git a/cpu/arm926ejs/kirkwood/kwcore.h b/cpu/arm926ejs/kirkwood/kwcore.h
> new file mode 100644
> index 0000000..0ddc6a8
> --- /dev/null
> +++ b/cpu/arm926ejs/kirkwood/kwcore.h
> @@ -0,0 +1,141 @@
...
> +/* Controler environment registers offsets */
> +#define KW_REG_MPP_CONTROL0 (0x10000)
> +#define KW_REG_MPP_CONTROL1 (0x10004)
> +#define KW_REG_MPP_CONTROL2 (0x10008)
> +#define KW_REG_MPP_CONTROL3 (0x1000C)
> +#define KW_REG_MPP_CONTROL4 (0x10010)
> +#define KW_REG_MPP_CONTROL5 (0x10014)
> +#define KW_REG_MPP_CONTROL6 (0x10018)
> +#define KW_REG_MPP_SMPL_AT_RST (0x10030)
> +#define KW_REG_CHIP_BOND (0x10034)
> +#define KW_REG_MPP_OUT_DRV_REG (0x100E0)
...
Use C structures instead of offset tables!
> +/*
> + * Macros
> + * CPU architecture dependent I/O read/write
> + */
> +#define KW_REG_WRITE(addr, data) \
> + ((*((volatile unsigned int*)(KW_REGISTER(addr)))) \
> + = ((unsigned int)WORD_SWAP((data))))
> +
> +#define KW_REG_READ(addr) \
> + WORD_SWAP(((*((volatile unsigned int*)(KW_REGISTER(addr))))))
> +
> +#define KW_REG_BITS_SET(adr, bits) (KW_REG_WRITE(adr, KW_REG_READ(adr)\
> + | ((unsigned int)WORD_SWAP(bits))))
> +
> +#define KW_REG_BITS_RESET(adr, bits) (KW_REG_WRITE(adr, KW_REG_READ(adr)\
> + & ~((unsigned int)WORD_SWAP(bits))))
Do not reinvent the wheel, do not pollute the code with tons of
redundand proprietary versions of maros that exist elsewhere.
> +#define KW_REG_READ_ASM(toReg, tmpReg, regOffs) \
> + ldr tmpReg, =KW_REGISTER(regOffs); \
> + ldr toReg, [tmpReg]; \
Indentation not by TAB. Please fix globally.
> --- /dev/null
> +++ b/cpu/arm926ejs/kirkwood/serial.c
...
> +/* This structure describes the registers offsets for one UART port/channel */
> +typedef struct kwUartPort {
> + u8 rbr; /* 0 = 0-3 */
> + u8 pad1[3];
> + u8 ier; /* 1 = 4-7 */
> + u8 pad2[3];
> + u8 fcr; /* 2 = 8-b */
> + u8 pad3[3];
> + u8 lcr; /* 3 = c-f */
> + u8 pad4[3];
> + u8 mcr; /* 4 = 10-13 */
> + u8 pad5[3];
> + u8 lsr; /* 5 = 14-17 */
> + u8 pad6[3];
> + u8 msr; /* 6 =18-1b */
> + u8 pad7[3];
> + u8 scr; /* 7 =1c-1f */
> + u8 pad8[3];
> +} kw_uart_port;
NAK.
We will definiteli not accept yet another version of 16550 compatible
UART register definitions. Please use the existing code (but wait
until Detlev Zundel's cleanup patch from today has been applied).
...
> diff --git a/cpu/arm926ejs/kirkwood/spi.c b/cpu/arm926ejs/kirkwood/spi.c
> new file mode 100644
> index 0000000..a334d95
> --- /dev/null
> +++ b/cpu/arm926ejs/kirkwood/spi.c
...
> + /*
> + * handle data in 8-bit chunks
> + * TBD: 2byte xfer mode to be enabled
> + */
> + while (bitlen > 4) {
> + debug_print("loopstart bitlen %d\n", bitlen);
> + tmpdout = 0;
> + if (1) { //bitlen <= 8) {
No C++ comments, please (fix globally).
> diff --git a/cpu/arm926ejs/kirkwood/timer.c b/cpu/arm926ejs/kirkwood/timer.c
> new file mode 100644
> index 0000000..4ab1a54
> --- /dev/null
> +++ b/cpu/arm926ejs/kirkwood/timer.c
...
> +ulong get_timer_masked(void)
> +{
> + ulong now = READ_TIMER;
> +
> + if (lastdec >= now) {
> + /* normal mode */
> + timestamp += lastdec - now;
> + } else {
> + /* we have an overflow ... */
> + timestamp +=
> + lastdec + (TIMER_LOAD_VAL / (CONFIG_SYS_TCLK / 1000)) - now;
Indentation not by TAB. Please check and fix globally.
> diff --git a/include/configs/kirkwood.h b/include/configs/kirkwood.h
> new file mode 100644
> index 0000000..eec04c2
> --- /dev/null
> +++ b/include/configs/kirkwood.h
> @@ -0,0 +1,46 @@
> +/*
> + * (C) Copyright 2009
> + * Marvell Semiconductor <www.marvell.com>
> + * Prafulla Wadaskar <prafulla at marvell.com>
> + *
> + * Header file for the Marvell's Feroceon CPU core.
NAK.
The include/configs/ directory is for board configuration files only.
CPU specific code has no place there.
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
"In Christianity neither morality nor religion come into contact with
reality at any point." - Friedrich Nietzsche
More information about the U-Boot
mailing list