Dear Wolfgang Wegner,
In message <1260378648-19232-1-git-send-email-w.weg...@astro-kom.de> you wrote:
> This patch adds support for ASTRO board(s) based on MCF5373L.
>
> Signed-off-by: Wolfgang Wegner <w.weg...@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: w...@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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
