Re: [U-Boot] [PATCH] add ASTRO MCF5373L board

17 Dec 2009

Dear Wolfgang Wegner,
In message 1260378648-19232-1-git-send-email-w.wegner@astro-kom.de you wrote:
...
This patch adds support for ASTRO board(s) based on MCF5373L.
Signed-off-by: Wolfgang Wegner w.wegner@astro-kom.de
There is another board (series) in the queue to be submitted, thus
I added a subdirectory for all our boards.
Makefile                         |   12 +
 board/astro/mcf5373l/Makefile    |   44 +++
 board/astro/mcf5373l/astro.h     |   39 +++
 board/astro/mcf5373l/config.mk   |   28 ++
 board/astro/mcf5373l/fpga.c      |  424 +++++++++++++++++++++++
 board/astro/mcf5373l/mcf5373l.c  |  204 +++++++++++
 board/astro/mcf5373l/u-boot.lds  |  142 ++++++++
 board/astro/mcf5373l/update.c    |  699 ++++++++++++++++++++++++++++++++++++++
 include/configs/astro_mcf5373l.h |  516 ++++++++++++++++++++++++++++
 9 files changed, 2108 insertions(+), 0 deletions(-)
 create mode 100644 board/astro/mcf5373l/Makefile
 create mode 100644 board/astro/mcf5373l/astro.h
 create mode 100644 board/astro/mcf5373l/config.mk
 create mode 100644 board/astro/mcf5373l/fpga.c
 create mode 100644 board/astro/mcf5373l/mcf5373l.c
 create mode 100644 board/astro/mcf5373l/u-boot.lds
 create mode 100644 board/astro/mcf5373l/update.c
 create mode 100644 include/configs/astro_mcf5373l.h
Entries for MAINTAINERS and MAKEALL missing.
...

diff --git a/Makefile b/Makefile
index 75b2c1e..924e210 100644
--- a/Makefile
+++ b/Makefile
@@ -2263,6 +2263,18 @@ M5485HFE_config :	unconfig
 TASREG_config :		unconfig
   @$(MKCONFIG) $(@:_config=) m68k mcf52x2 tasreg esd
+astro_mcf5373l_config \
+astro_mcf5373l_ram_config :	unconfig

@if [ "$@" = "astro_mcf5373l_ram_config" ] ; then \
echo "#define CONFIG_MONITOR_IS_IN_RAM"	>> $(obj)include/config.h ; \


echo "TEXT_BASE = 0x40020000" > $(obj)board/astro/mcf5373l/config.tmp ; \


$(XECHO) "... for RAM boot ..." ; \


else \
echo "TEXT_BASE = 0x00000000" > $(obj)board/astro/mcf5373l/config.tmp ; \


$(XECHO) "... for FLASH boot ..." ; \


fi
@$(MKCONFIG) -a astro_mcf5373l m68k mcf532x mcf5373l astro

Please keep lists sorted, and don't add such scripting to the
Makefile. It is not needed any more.
...
diff --git a/board/astro/mcf5373l/astro.h b/board/astro/mcf5373l/astro.h
new file mode 100644
index 0000000..9478787
--- /dev/null
+++ b/board/astro/mcf5373l/astro.h
...
...
+typedef struct _s_karten_id {

char Kartentyp;
char Hardwareversion;
char Softwareversion;
char SoftwareSubversion;	/* " ","a",.."z" */
char FPGA_Version_Alt;
char FPGA_Version_Xil;

+} s_karten_id;
Variable names must be all lower case. Please fix globally.
...
+typedef struct {

unsigned char mode;
unsigned char deviation;
unsigned short freq;

+} __attribute__ ((packed)) s_output_params;
And please decide for one language. Don;t use half german and half
English names. Note that English is striclty preferred.
...
diff --git a/board/astro/mcf5373l/fpga.c b/board/astro/mcf5373l/fpga.c
new file mode 100644
index 0000000..eb1bf49
--- /dev/null
+++ b/board/astro/mcf5373l/fpga.c
...
...
+/*


Altera/Xilinx FPGA configuration support for the ASTRO "URMEL" board


*/


+#include <common.h>
+#include <watchdog.h>
+#include <altera.h>
+#include <ACEX1K.h>
+#include <spartan3.h>
+#include <command.h>
+#include <asm/immap_5329.h>
+#include "fpga.h"



+DECLARE_GLOBAL_DATA_PTR;



+int altera_pre_fn (int cookie)
+{

volatile gpio_t *gpiop = (volatile gpio_t *)MMAP_GPIO;


+/* first, set the required pins to GPIO function */
Incorrect indentation. Please fix globally.
...

/* PAR_T0IN -> GPIO */
gpiop->par_timer &= 0xfc;

Please use I/O accessors to access device registers. Please fix
globally.
...



+/* writes the complete buffer to the FPGA

writing the complete buffer in one function is much faster,
then calling it for every bit */

Incorrect multiline comment. Please fix globally.
...
+/*


Test the state of the active-low FPGA INIT line.  Return 1 on INIT



asserted (low).


*/

+int xilinx_init_fn (int cookie)
+{

unsigned char state;
volatile gpio_t *gpiop = (volatile gpio_t *)MMAP_GPIO;

state = (gpiop->ppd_pwm & 0x08) >> 3;
if (state)
return 0;


else
return 1;



Simplify:
return ((gpiop->ppd_pwm & 0x08) != 0);
(but use I/O accessor instead.
...
--- /dev/null
+++ b/board/astro/mcf5373l/mcf5373l.c
...
...

return CONFIG_SYS_SDRAM_SIZE * 1024 * 1024;

Use get_ram_size() ?
...
+int testdram (void)
+{

printf ("DRAM test not implemented!\n");

return (0);

+}
We already have a number of memory tests. Don't reinvent the wheel.
...
+void astro_put_char (char ch)
+{

volatile uart_t *uart;
int i;

uart = (volatile uart_t *)(MMAP_UART0);
/* Wait for last character to go. */
for (i = 0; (i < 0x10000); i++)
if (uart->usr & UART_USR_TXRDY)


	break;



Braces needed for multiline statement. And you probably want to use a
better timeout.
...
diff --git a/board/astro/mcf5373l/u-boot.lds b/board/astro/mcf5373l/u-boot.lds
new file mode 100644
index 0000000..a9a4e0a
--- /dev/null
+++ b/board/astro/mcf5373l/u-boot.lds
Do you really need a board specific linker script?
...
diff --git a/board/astro/mcf5373l/update.c b/board/astro/mcf5373l/update.c
new file mode 100644
index 0000000..ac57816
--- /dev/null
+++ b/board/astro/mcf5373l/update.c
...
...
+#define CMD_INIT		0
+#define CMD_NEW			1
+#define CMD_TX			2
+#define CMD_RX			3
+#define CMD_OK			4
+#define CMD_FAIL		5
+#define CMD_CH_RX		6
+#define CMD_TX_END		7
+#define CMD_RX_END		8
+#define CMD_TX_RDY		9
+#define CMD_RX_RDY		10
+#define CMD_RX_ERROR		11
Looks as if you wanted to use an enum here?
...
+#define	F_Daten_Speichern	 	0x00	/* save parameters on card */
+#define	F_Daten_Lesen		 	0x01	/* read parameters */
+#define	F_Karteninfo_Lesen 	 	0x10	/* read 5 Byte card information */
+#define	F_Kartenfehler_Lesen	 	0x11	/* read 4 Byte card error */
+#define	F_Transparente_Daten	 	0x20	/* length byte + data */



+#define SC_Prepare_for_Flash_Data	0xc0
+#define SC_Flash_Data			0xc1
+#define SC_Clear_CRC			0xc2
+#define SC_Check_CRC			0xc3
+#define SC_Restart			0xc4
Macros use all caps names. And decide for _one_ language.
...
+void do_crc (unsigned char data)
+{
Cool. Yet another CRC function :-(
...
+int dez2hex (int in)
+{

int out;

out = 16 * (in / 10);
out += (in % 10);
return out;

+}
Get rid of this and use proper conversion routines.
...

if (flash_prog_stat == FL_STAT_IDLE) {
if (((flash_prog_count & 0x1ffff) == 0) &&


    ((flash_prog_count >> 17) == flash_sect_count)) {


	flash_protect (FLAG_PROTECT_CLEAR, fl_adr,


			fl_adr + 0x1ffff, flash_info);


	s = flash_erase_nb (flash_info, fl_sector);


	flash_prog_stat = FL_STAT_ERASE;


}


else if ((flash_data_count >= (flash_prog_count + 64))



Move 'else' on previous line.
...

	   || ((int_command & INT_CMD_WRITE_FLASH)


	       && (flash_data_count > flash_prog_count))) {


	s = write_buff_nb (flash_info,


			   flash_buffer +


			   (flash_prog_count & 0xffff),


			   fl_adr, 64);


	flash_prog_stat = FL_STAT_PROG;


}


}
else if (flash_prog_stat == FL_STAT_ERASE) {

Ditto.
...

s = flash_full_status_check_nb (flash_info, fl_sector,


				0, "erase");


if (s != ERR_BUSY) {


	if (s == ERR_OK) {


		flash_sect_count++;


		flash_prog_stat = FL_STAT_IDLE;


	}


	else



And again. Please fix indentation globally.
...

if (s != ERR_BUSY) {


	if (s == ERR_OK) {


		flash_prog_stat = FL_STAT_IDLE;


		for (i = 0; i < 64; i++)


			if (flash_buffer


			    [(flash_prog_count + i)


			     & 0xffff] !=


			    *((unsigned char *)fl_adr +


			      i))


				flash_prog_stat =


				    FL_STAT_ERROR;



This looks very much unreadable.
Quoting "Documentation/CodingStyle":
Now, some people will claim that having 8-character indentations makes
the code move too far to the right, and makes it hard to read on a
80-character terminal screen.  The answer to that is that if you need
more than 3 levels of indentation, you're screwed anyway, and should fix 
your program.
...

if (env_string != NULL) {
i = (int)simple_strtol (env_string, NULL, 10);


Karteninformation.Hardwareversion = dez2hex (i);



Why are you doing this? Why not simply
Karteninformation.Hardwareversion =
    		simple_strtol(env_string, NULL, 16);
?
Also mind that the CodingStyle does not allow for spaces between
function name and '('. Please fix globally.
...
...

	/* set high baud rate */


	rs_serial_init (0, 115200);


	// Reset the Receive Byte Counter



No C++ comments, please.
...

if (select) {



+/***************************************************************************/
+/**     UART RX Routine                                                   **/
+/***************************************************************************/
Indentation wrong, incorrect multiline comment. Please fix globally.
...
...
+U_BOOT_CMD (update, 2, 1, do_update,

   "update  - update function information\n",


   "\n    - print information for update\n"


   "update N\n    - print information for update # N\n");



Check oif this help message really looks what you think it should look
like. For example, the last '\n' is probably wrong.
...
diff --git a/include/configs/astro_mcf5373l.h b/include/configs/astro_mcf5373l.h
new file mode 100644
index 0000000..2ce80f8
--- /dev/null
+++ b/include/configs/astro_mcf5373l.h
...
...
+/* ---


Enable use of Ethernet







*/


+/* ethernet could be useful for debugging */
+/* #define FEC_ENET */
Do not add dead code.
...
...
+/* ---


set "#if 0" to "#if 1" if (Hardware)-WATCHDOG should be enabled & change



timeout acc. to your needs



#define CONFIG_WATCHDOG_TIMEOUT x , x is timeout in milliseconds, e. g. 10000



for 10 sec







*/


+#ifndef CONFIG_MONITOR_IS_IN_RAM
+#define CONFIG_WATCHDOG
+#define CONFIG_WATCHDOG_TIMEOUT 3355	/* timeout in milliseconds */
+#endif
Which "#if 0" are you referring to in the comment?
...
+#define CONFIG_EXTRA_ENV_SETTINGS			\

"loaderversion=11\0"				\
"card_id="QUOTEME(ASTRO_ID)"\0"			\
"alterafile=0\0"				\
"xilinxfile=0\0"				\
"xilinxload=imxtract 0x540000 $xilinxfile 0x41000000&&fpga load 0 0x41000000 $filesize\0" \
"alteraload=imxtract 0x6c0000 $alterafile 0x41000000&&fpga load 1 0x41000000 $filesize\0" \

Lines too long, Please fix globally.
...
+#define CONFIG_ETHADDR 00:00:00:00:00:09	/* default ethernet MAC addr. */
+#define CONFIG_IPADDR 192.168.100.2		/* default board IP address */
+#define CONFIG_SERVERIP 192.168.100.1	/* default tftp server IP address */
NAK.
Remove these settings, and never ever use bogus or stolen MAC
addresses.
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
If it happens once, it's a bug.
If it happens twice, it's a feature.
If it happens more than twice, it's a design philosophy.

    