[U-Boot] [PATCH v2 8/9] AVR32: CPU support for AT32UC3A0xxx CPUs

[Wolfgang Denk]

Dear Gunnar Rangoy,

In message <b47b53bb44d0d937f9a40249c1cd668640aec400.1232710611.git.gunnar at rangoy.com> you wrote:
> This patch adds support for the AT32UC3A0xxx chips.
...
> +++ b/cpu/at32uc/at32uc3a0xxx/portmux.c
> @@ -0,0 +1,154 @@
...
> +	portmux_select_peripheral(PORTMUX_PORT(0),
> +						0x0003C000 |
> +						0x1E000000, PORTMUX_FUNC_C, 0);
> +	portmux_select_peripheral(PORTMUX_PORT(1),
> +						0x00000010 |
> +						0x00007C00 |
> +						0x00030000 |
> +						0xE0000000, PORTMUX_FUNC_C, 0);
> +	portmux_select_peripheral(PORTMUX_PORT(2),
> +						0xFFFFFFC0, PORTMUX_FUNC_A, 0);
> +	portmux_select_peripheral(PORTMUX_PORT(3),
> +						0x00003FFF, PORTMUX_FUNC_A, 0);
> +}

It would be nice if you used readable names instead of all these
magic hardcoded constants.

> +++ b/cpu/at32uc/at32uc3a0xxx/sm.h
> @@ -0,0 +1,247 @@
> +/*
> + * Register definitions for System Manager
> + */
> +#ifndef __CPU_AT32UC_SM_H__
> +#define __CPU_AT32UC_SM_H__
> +
> +/* SM register offsets */
> +/* PM starts at 0xFFFF0C00 */
> +#define SM_PM_REGS_OFFSET			0x0c00
> +#define SM_PM_MCCTRL				(SM_PM_REGS_OFFSET + 0x0000)
> +#define SM_PM_CKSEL				(SM_PM_REGS_OFFSET + 0x0004)
> +#define SM_PM_CPU_MASK				(SM_PM_REGS_OFFSET + 0x0008)
> +#define SM_PM_HSB_MASK				(SM_PM_REGS_OFFSET + 0x000c)
> +#define SM_PM_PBA_MASK				(SM_PM_REGS_OFFSET + 0x0010)
> +#define SM_PM_PBB_MASK				(SM_PM_REGS_OFFSET + 0x0014)
> +#define SM_PM_PLL0				(SM_PM_REGS_OFFSET + 0x0020)
> +#define SM_PM_PLL1				(SM_PM_REGS_OFFSET + 0x0024)
> +#define SM_PM_OSCCTRL0				(SM_PM_REGS_OFFSET + 0x0028)
...

Instead of using offsets everywhere I would appreciate if the code
could be changed to use C structs instead (like we do in PPC land).


> +int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> +{
> +	/* This will reset the CPU core, caches, MMU and all internal busses */
> +	__builtin_mtdr(8, 1 << 13);	/* set DC:DBE */
> +	__builtin_mtdr(8, 1 << 30);	/* set DC:RES */
> +
> +	/* Flush the pipeline before we declare it a failure */
> +	asm volatile("sub   pc, pc, -4");
> +
> +	return -1;
> +}

I read this as if you just reset the CPU "internal" stuff. Sorry for
asking stupid questions, I don't know this architecture at all, but:
Will external chips be reset this way, too?  Or how do you make sure
that external peripherals get properly reset?

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
365 Days of drinking Lo-Cal beer.                       = 1 Lite-year