[U-Boot] [PATCH 01/10] mx27: basic cpu support

[Wolfgang Denk <wd@denx.de>]

Dear Ilya,

in message <1241634633-13917-2-git-send-email-yanok@emcraft.com> you wrote:
> This patch adds generic code to support Freescale's i.MX27 SoCs.
...

> +static ulong clk_in_26m(void)
> +{
> +	if (CSCR & CSCR_OSC26M_DIV1P5) {
> +		/* divide by 1.5 */
> +		return 26000000 / 1.5;

We definitely do not allow any FP use in U-Boot.

> +	} else {
> +		/* divide by 1 */
> +		return 26000000;

divide by 1 ???


> +#if defined(CONFIG_DISPLAY_CPUINFO)
> +int print_cpuinfo (void)
> +{
> +       printf("CPU:   Freescale i.MX27 at %llu MHz\n",
> +               lldiv(imx_get_mpllclk(), 1000000));

Please use strmhz() to print frequencies.

> +       printf("\n");

No need to waste another function call - just add this to the previous
format element.

> +void imx_gpio_mode(int gpio_mode)
> +{
> +	unsigned int pin = gpio_mode & GPIO_PIN_MASK;
> +	unsigned int port = (gpio_mode & GPIO_PORT_MASK) >> GPIO_PORT_SHIFT;
> +	unsigned int ocr = (gpio_mode & GPIO_OCR_MASK) >> GPIO_OCR_SHIFT;
> +	unsigned int aout = (gpio_mode & GPIO_AOUT_MASK) >> GPIO_AOUT_SHIFT;
> +	unsigned int bout = (gpio_mode & GPIO_BOUT_MASK) >> GPIO_BOUT_SHIFT;
> +	unsigned int tmp;
> +
> +	/* Pullup enable */
> +	if(gpio_mode & GPIO_PUEN)
> +		PUEN(port) |= (1 << pin);
> +	else
> +		PUEN(port) &= ~(1 << pin);

This smells as if these were pointer accesses using register offsets
instead of I/O accessor calls using C structs?

You probably want to use the respective clrbits*() / setbits*() macros
here?

...
> +void reset_cpu (ulong ignored)
> +{
> +	/* Disable watchdog and set Time-Out field to 0 */
> +	WCR = 0x00000000;
> +
> +	/* Write Service Sequence */
> +	WSR = 0x00005555;
> +	WSR = 0x0000AAAA;

Again: please use I/O accessor calls; also for the rest of the file
and the other patches.

...
> diff --git a/include/asm-arm/arch-mx27/imx-regs.h b/include/asm-arm/arch-mx27/imx-regs.h
> new file mode 100644
> index 0000000..a856f7e
...
> +# ifndef __ASSEMBLY__
> +# define __REG(x)	(*((volatile u32 *)(x)))
> +# define __REG16(x)     (*(volatile u16 *)(x))
> +# define __REG2(x,y)    (*(volatile u32 *)((u32)&__REG(x) + (y)))
> +
> +extern void imx_gpio_mode (int gpio_mode);
> +
> +# else
> +#  define __REG(x) (x)
> +#  define __REG16(x) (x)
> +#  define __REG2(x,y) ((x)+(y))
> +#endif
> +
> +#define IMX_IO_BASE		0x10000000
> +
> +#define IMX_AIPI1_BASE             (0x00000 + IMX_IO_BASE)
> +#define IMX_WDT_BASE               (0x02000 + IMX_IO_BASE)
> +#define IMX_TIM1_BASE              (0x03000 + IMX_IO_BASE)
> +#define IMX_TIM2_BASE              (0x04000 + IMX_IO_BASE)
> +#define IMX_TIM3_BASE              (0x05000 + IMX_IO_BASE)
> +#define IMX_UART1_BASE             (0x0a000 + IMX_IO_BASE)
> +#define IMX_UART2_BASE             (0x0b000 + IMX_IO_BASE)
> +#define IMX_UART3_BASE             (0x0c000 + IMX_IO_BASE)
> +#define IMX_UART4_BASE             (0x0d000 + IMX_IO_BASE)
> +#define IMX_I2C1_BASE              (0x12000 + IMX_IO_BASE)
> +#define IMX_GPIO_BASE              (0x15000 + IMX_IO_BASE)
> +#define IMX_TIM4_BASE              (0x19000 + IMX_IO_BASE)
> +#define IMX_TIM5_BASE              (0x1a000 + IMX_IO_BASE)
> +#define IMX_UART5_BASE             (0x1b000 + IMX_IO_BASE)
> +#define IMX_UART6_BASE             (0x1c000 + IMX_IO_BASE)
> +#define IMX_I2C2_BASE              (0x1D000 + IMX_IO_BASE)
> +#define IMX_TIM6_BASE              (0x1f000 + IMX_IO_BASE)
> +#define IMX_AIPI2_BASE             (0x20000 + IMX_IO_BASE)
> +#define IMX_PLL_BASE               (0x27000 + IMX_IO_BASE)
> +#define IMX_SYSTEM_CTL_BASE        (0x27800 + IMX_IO_BASE)
> +#define IMX_IIM_BASE               (0x28000 + IMX_IO_BASE)
> +#define IMX_FEC_BASE               (0x2b000 + IMX_IO_BASE)

NAK. We do not accept device I/O through pointers; please use C
structs to describe the hardware and use I/O accessor calls.

> +/* AIPI */
> +#define AIPI1_PSR0	__REG(IMX_AIPI1_BASE + 0x00)
> +#define AIPI1_PSR1	__REG(IMX_AIPI1_BASE + 0x04)
> +#define AIPI2_PSR0	__REG(IMX_AIPI2_BASE + 0x00)
> +#define AIPI2_PSR1	__REG(IMX_AIPI2_BASE + 0x04)
> +
> +/* System Control */
> +#define FMCR	__REG(IMX_SYSTEM_CTL_BASE + 0x14)
> +#define GPCR	__REG(IMX_SYSTEM_CTL_BASE + 0x18)
> +#define WBCR	__REG(IMX_SYSTEM_CTL_BASE + 0x1C)

NAK, for this and all similar code.


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
It is better to marry than to burn.
                                - Bible ``I Corinthians'' ch. 7, v. 9