[U-Boot] [PATCH V2 1/3] Initial support for Marvell Orion5x SoC

[Wolfgang Denk]

Dear Albert Aribaud,

In message <1258239796-21528-2-git-send-email-albert.aribaud at free.fr> you wrote:
> This patch adds support for the Marvell Orion5x SoC.
> It has no use alone, and must be followed by a patch
> to add Orion5x support for serial, then support for
> the ED Mini V2, an Orion5x-based board from LaCie.
...

> diff --git a/cpu/arm926ejs/orion5x/timer.c b/cpu/arm926ejs/orion5x/timer.c
> new file mode 100644
...

> +#define READ_TIMER \
> +	(readl(CNTMR_VAL_REG(UBOOT_CNTR)) / (CONFIG_SYS_TCLK / 1000))
...
> +	/* reset time */
> +	lastdec = READ_TIMER;

Macros resembling functions should look like functions, i. e. have
parens. Also please note that generally, inline functions are
preferable to macros resembling functions.

...
> +#define UBOOT_CNTR_VAL readl(CNTMR_VAL_REG(UBOOT_CNTR))

Ditto.

> +void udelay(unsigned long usec)
> +{
> +	uint current;
> +	ulong delayticks;
> +
> +	current = readl(CNTMR_VAL_REG(UBOOT_CNTR));

Why do you add the #define above when you then don't use it?

> +	delayticks = (usec * (CONFIG_SYS_TCLK / 1000000));
> +
> +	if (current < delayticks) {
> +		delayticks -= current;
> +		while (readl(CNTMR_VAL_REG(UBOOT_CNTR)) < current)

Ditto.

diff --git a/cpu/arm926ejs/orion5x/mpp.c b/cpu/arm926ejs/orion5x/mpp.c
new file mode 100644
index 0000000..f341747
--- /dev/null
+++ b/cpu/arm926ejs/orion5x/mpp.c
...
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.

NAK. License must be "v2 or any later version". Ditto for some other
files.

> diff --git a/include/asm-arm/arch-orion5x/88f5182.h b/include/asm-arm/arch-orion5x/88f5182.h
> new file mode 100644
> index 0000000..b16b23f
> --- /dev/null
> +++ b/include/asm-arm/arch-orion5x/88f5182.h

88f5182 is a terrible file name. Can you not come up with something
more descriptive, please?

> diff --git a/include/asm-arm/arch-orion5x/gpio.h b/include/asm-arm/arch-orion5x/gpio.h
> new file mode 100644
> index 0000000..58592ad
> --- /dev/null
> +++ b/include/asm-arm/arch-orion5x/gpio.h
...
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2.  This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.

NAK. See above.

> +#define GPIO_MAX		26
> +#define GPIO_OUT(pin)		(ORION5X_GPIO0_BASE + 0x00)
> +#define GPIO_IO_CONF(pin)	(ORION5X_GPIO0_BASE + 0x04)
> +#define GPIO_BLINK_EN(pin)	(ORION5X_GPIO0_BASE + 0x08)
> +#define GPIO_IN_POL(pin)	(ORION5X_GPIO0_BASE + 0x0c)
> +#define GPIO_DATA_IN(pin)	(ORION5X_GPIO0_BASE + 0x10)
> +#define GPIO_EDGE_CAUSE(pin)	(ORION5X_GPIO0_BASE + 0x14)
> +#define GPIO_EDGE_MASK(pin)	(ORION5X_GPIO0_BASE + 0x18)
> +#define GPIO_LEVEL_MASK(pin)	(ORION5X_GPIO0_BASE + 0x1c)

NAK. Please make this a C struct.

> +/*
> + * Orion5x-specific GPIO API
> + */

Why do we need an Orion5x-specific GPIO API? Can't we use something
that is more general?

> diff --git a/include/asm-arm/arch-orion5x/mpp.h b/include/asm-arm/arch-orion5x/mpp.h
> new file mode 100644
> index 0000000..31fade7
> --- /dev/null
> +++ b/include/asm-arm/arch-orion5x/mpp.h
...
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2.  This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.

NAK. See above.


> +#define MPP(_num, _sel, _in, _out, _F5182, _F5281) ( \
> +	/* MPP number */		((_num) & 0xff) | \
> +	/* MPP select value */		(((_sel) & 0xf) << 8) | \
> +	/* may be input signal */	((!!(_in)) << 12) | \
> +	/* may be output signal */	((!!(_out)) << 13) | \
> +	/* available on F5182 */	((!!(_F5182)) << 14) | \
> +	/* available on F5281 */	((!!(_F5182)) << 15))

Comments follow the code, not vice versa.


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
The night sky over the planet Krikkit is the least interesting  sight
in the entire Universe.
                 - Douglas Adams _Life, the Universe, and Everything_