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

[Gunnar Rangøy]

On Fri, Jan 23, 2009 at 5:00 PM, Wolfgang Denk <wd at denx.de> wrote:
> 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.

Each bit in the masks represents a single pin on the microcontroller.
The readable names would therefore become something like:
#define EBI_PINS_PORT0 (0x0003C000 | 0x1E000000)
#define EBI_PINS_PORT1 (0x00000010 | 0x00007C00 | ...)
I'm not certain that that would make it much more readable. The
bitmasks are more or less based on reading the datasheet and counting
which pins are used.

Maybe adding a comment before the code would be just as useful?


>> +++ 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).

It was done like this because it was done in the existing AVR32 code
which we based our work on. Since we used parts of that code, it made
most sense to use the same coding style.

We can change it if you think it is necessary.

>
>> +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?

As most of the needed functionality is embedded in the microcontroller,
there are very few external peripherals used by U-Boot. Apart from
external memory, and oscillator, and level-shifters for the serial-port,
there is only the ethernet PHY, and that one shouldn't need a reset.


Gunnar Rangøy,
99360699