Re: [U-Boot] [PATCH] Marvell Kirkwood family SOC support

3 Apr 2009

Dear Prafulla Wadaskar,
In message 1238798370-9245-2-git-send-email-prafulla@marvell.com you wrote:
...
From: prafulla_wadaskar prafulla@marvell.com
Kirkwood family controllers are highly integrated SOCs
based on Feroceon-88FR131/Sheeva-88SV131 cpu core.
SOC versions supported:-

88F6281-Z0       define CONFIG_KW88F6281_Z0
88F6281-A0       define CONFIG_KW88F6281_A0
88F6192-A0       define CONFIG_KW88F6192_A0

Other supported features:-

Doimage utility needed to create binaries with
bootROM header
get_random_hex() fucntion
SPI port controller driver
PCI Express port initialization

Contributors:
Yotam Admon yotam@marvell.com
Michael Blostein <michaelbl@marvell.com
Signed-off-by: prafulla_wadaskar prafulla@marvell.com
Reviewed by: Ronen Shitrit rshitrit@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@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@denx.de
"In Christianity neither morality nor religion come into contact with
reality at any point."                          - Friedrich Nietzsche

    