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

Wolfgang Denk
2009-01-23T16:00:12Z

Dear Gunnar Rangoy,

In message <b47b53bb44d0d937f9a40249c1cd668640aec400.1232710611.git.gunnar@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