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

Wolfgang Denk
2009-11-18T22:17:16Z

Dear Albert Aribaud,

In message <1258239796-21528-2-git-send-email-albert.aribaud@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@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_