Dear Gunnar Rangoy,

In message 
<b47b53bb44d0d937f9a40249c1cd668640aec400.1232710611.git.gun...@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: w...@denx.de
365 Days of drinking Lo-Cal beer.                       = 1 Lite-year
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to