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

[Wolfgang Denk]

Dear Wolfgang Wegner,

In message <1260378648-19232-1-git-send-email-w.wegner at astro-kom.de> you wrote:
> This patch adds support for ASTRO board(s) based on MCF5373L.
> 
> Signed-off-by: Wolfgang Wegner <w.wegner at 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 at 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.