[U-Boot] [PATCH 1/3] arm: Added support for MB86R01 'Jade' SoC

[Wolfgang Denk]

Dear Matthias Weisser,

In message <1247039227-17910-2-git-send-email-matthias.weisser at graf-syteco.de> you wrote:
> This patch adds support for MB86R01 'Jade' SoC from Fujitsu
...
> +/* nothing really to do with interrupts, just starts up a counter. */
> +int timer_init(void)
> +{
> +	*(volatile ulong *)(TIMER_BASE + 0) = TIMER_LOAD_VAL;
> +	*(volatile ulong *)(TIMER_BASE + 8) = 0x86;

Here and everyhere else: please use accessor functions instead of
(volatile) register assignments.

> +unsigned long long get_ticks(void)
> +{
> +	ulong now = READ_TIMER;
> +
> +	if (now <= lastdec)	/* normal mode (non roll) */
> +		/* move stamp forward with absolut diff ticks */
> +		timestamp += (lastdec - now);
> +	else			/* we have rollover of incrementer */

Please add braces for multiline branches.

> +void udelay(unsigned long usec)
> +{
> +	unsigned long long tmp;
> +	ulong tmo;
> +
> +	tmo = usec_to_tick(usec);
> +	tmp = get_ticks() + tmo;	/* get current timestamp */
> +
> +	while (get_ticks() < tmp)	/* loop till event */
> +		/*NOP*/;

What about the timestamp wrapping around?

> +ulong get_tbclk(void)
> +{
> +	ulong tbclk;
> +
> +	tbclk = CONFIG_SYS_HZ;
> +	return tbclk;
> +}

Why not simply "return CONFIG_SYS_HZ;" ?

> diff --git a/include/asm-arm/arch-jade/jade.h b/include/asm-arm/arch-jade/jade.h
> new file mode 100755
> index 0000000..c2b28a2
> --- /dev/null
> +++ b/include/asm-arm/arch-jade/jade.h
...
> +#ifndef JADE_H
> +#define JADE_H
> +
> +typedef	volatile unsigned int	JREG;	/* Hardware register definition */

This is not needed / allowed/ Please use accessor functions.

> +/*
> + * Physical Address Defines
> + */
> +#define JADE_GDC_PHYS_BASE	0xf1fc0000		/* GDC phys */
> +#define JADE_GDC_PHYS_DISP_BASE	0xf1fd0000		/* GDC DisplayBase phys */
> +#define JADE_CCNT_PHYS_BASE	0xfff42000		/* Chip Control Module */
> +#define JADE_CAN0_PHYS_BASE	0xfff54000		/* CAN 0 phys */
> +#define JADE_CAN1_PHYS_BASE	0xfff55000		/* CAN 1 phys */
> +#define JADE_I2C0_PHYS_BASE	0xfff56000		/* I2C 0 phys */
> +#define JADE_I2C1_PHYS_BASE	0xfff57000		/* I2C 1 phys */
> +#define JADE_EHCI_PHYS_BASE	0xfff80000		/* EHCI phys */
> +#define JADE_OHCI_PHYS_BASE	0xfff81000		/* OHCI phys */
> +#define JADE_IRC1_PHYS_BASE	0xfffb0000		/* Jade cascaded Interrupt Controller phys */

Line too long (many more too long lines, please check globally).

> +#define JADE_TIMER_PHYS_BASE	0xfffe0000		/* Counter/Timers JADE phys */
> +#define JADE_UART0_PHYS_BASE	0xfffe1000		/* UART 0 phys */
> +#define JADE_UART1_PHYS_BASE	0xfffe2000		/* UART 1 phys */
> +#define JADE_IRCE_PHYS_BASE	0xfffe4000		/* Extended Interrupt Controller */
> +#define JADE_CRG_PHYS_BASE	0xfffe7000		/* Clock Reset Generator */
> +#define JADE_IRC0_PHYS_BASE	0xfffe8000		/* Jade Interrupt Controller phys */
> +#define JADE_GPIO_PHYS_BASE	0xfffe9000		/* GPIO phys */

Please do not use register addresses directly - declare a C structure,
and use typed variables. This applies not only here, but globally.


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
God grant me the senility to accept the things I cannot  change,  The
frustration  to  try to change things I cannot affect, and the wisdom
to tell the difference.