[U-Boot] [PATCH 1/5] those files are jz4740 base files

[Wolfgang Denk]

Dear Xiangfu Liu,

In message <1289446657-12499-2-git-send-email-xiangfu at openmobilefree.net> you wrote:
> From: Xiangfu Liu <xiangfu at sharism.cc>

Please use a more descriptive subject.

Explain what jz4740 is; also explain what xburst is. Whos is be=hind
it, what it's used for, etc.


> diff --git a/arch/mips/cpu/xburst/interrupts.c b/arch/mips/cpu/xburst/interrupts.c
> new file mode 100644
> index 0000000..87f7a9f
> --- /dev/null
> +++ b/arch/mips/cpu/xburst/interrupts.c
...
> +void enable_interrupts(void)
> +{
> +}
> +
> +int disable_interrupts(void)
> +{
> +	return 0;
> +}

It does not seem you are using interrupts? If so, then omit this file
all together.

> diff --git a/arch/mips/cpu/xburst/jz4740.c b/arch/mips/cpu/xburst/jz4740.c
> new file mode 100644
> index 0000000..b8e9a15
> --- /dev/null
> +++ b/arch/mips/cpu/xburst/jz4740.c
...
> +	pllout2 = (cfcr & CPM_CPCCR_PCS) ? CONFIG_SYS_CPU_SPEED : (CONFIG_SYS_CPU_SPEED / 2);

Line too long. Please fix globally.

> +	/* Init USB Host clock, pllout2 must be n*48MHz */
> +	REG_CPM_UHCCDR = pllout2 / 48000000 - 1;

What's REG_CPM_UHCCDR? This symbol is not defined anywhere in this
patch, which means that none of this code would ever compile.

If you split patches, please make sure they contain complete,
compilable code to make the commit bisectable.


To me this looks as if you are implementing these register accesses as
plain assignments to a volatile pointer, probably based on a register
base address plus offset calculation.  Please note that we will NOT
accept any such code in U-Boot.  Please use C structs to describe
functional units, and use proper I/O accessors to access any such
registers.

Please fix globally.

> +static void calc_clocks(void)
> +{
> +	DECLARE_GLOBAL_DATA_PTR;

This does not work. DECLARE_GLOBAL_DATA_PTR must never be used in
function scope; it must be used in file scope.

> +static void rtc_init(void)
> +{
> +	while ( !__rtc_write_ready()) ;

Please move semicolon to next line.

> +#if 0
> +	unsigned long rtcsta;
> +
> +	while ( !__rtc_write_ready())
> +		;
> +	rtcsta = REG_RTC_HWRSR;
> +	while ( !__rtc_write_ready())
> +		;
> +	if (rtcsta & 0x33) {
> +		if (rtcsta & 0x10) {
> +			while ( !__rtc_write_ready())
> +				;
> +			REG_RTC_RSR = 0x0;
> +		}
> +		while ( !__rtc_write_ready())
> +			;
> +		REG_RTC_HWRSR = 0x0;
> +	}
> +#endif

Please remove all such dead code.

> diff --git a/arch/mips/cpu/xburst/jz_serial.c b/arch/mips/cpu/xburst/jz_serial.c
> new file mode 100644
> index 0000000..c5cc28e
> --- /dev/null
> +++ b/arch/mips/cpu/xburst/jz_serial.c
> @@ -0,0 +1,139 @@
> +/*
> + * Jz47xx UART support
> + *
> + * Hardcoded to UART 0 for now
> + * Options also hardcoded to 8N1
> + *
> + *  Copyright (c) 2005
> + *  Ingenic Semiconductor, <jlwei at ingenic.cn>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include <config.h>
> +#include <common.h>
> +
> +#if defined(CONFIG_JZ4740)
> +#include <asm/jz4740.h>
> +#endif
> + 
> +#undef UART_BASE
> +#ifndef CONFIG_SYS_UART_BASE
> +#define UART_BASE  UART0_BASE
> +#else
> +#define UART_BASE  CONFIG_SYS_UART_BASE
> +#endif

This initial "#undef UART_BASE" looks fishy to me.  Please explain why
this is unavoidable here.

> +/******************************************************************************
> +*
> +* serial_init - initialize a channel

Incorrect multiline comment style. Please fix globally.

> +void serial_puts (const char *s)
> +{
> +	while (*s) {
> +		serial_putc (*s++);
> +	}

No braces needed for single line statements.  Please fix globally.

> +int serial_getc (void)
> +{
> +	volatile u8 *uart_rdr = (volatile u8 *)(UART_BASE + OFF_RDR);
> +
> +	while (!serial_tstc());

Please use a consistent style. Move the semicolon to the next line.
Please fix globally.

> +void __udelay (unsigned long usec)
> +{
> +	ulong tmo,tmp;
> +
> +	/* normalize */
> +	if (usec >= 1000) {
> +		tmo = usec / 1000;
> +		tmo *= TIMER_HZ;
> +		tmo /= 1000;
> +	}
> +	else {

Incorrect brace style.

> +		if (usec >= 1) {
> +			tmo = usec * TIMER_HZ;
> +			tmo /= (1000*1000);
> +		}
> +		else

Again. Please fix globally.

> +usb_boot:
> +	//--------------------------------------------------------------
> +	// Initialize PLL: set ICLK to 84MHz and HCLK to 42MHz.
> +	//--------------------------------------------------------------
> +	la	$9, 0xB0000000		// CPCCR: Clock Control Register
> +	la	$8, 0x42041110		// I:S:M:P=1:2:2:2
> +	sw	$8, 0($9)

C++ comments are not allowed in U-Boot. Please fix globally.

Would it be possible to rewrite this file in C?


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
People have one thing in common: they are all different.