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

[Wolfgang Denk]

Dear =?ISO-8859-1?Q?Gunnar_Rang=F8y?=,

In message <e1b3d0a60901260726i422cfe85s7c5a35745eedc60f at mail.gmail.com> you wrote:
>
> >> +     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 | ...)

Um, no. You didn't get it. You use magic numbers again.

That should be some

#define FOO_PIN_XXX	0x0003C000
#define FOO_PIN_YYY	0x1E000000
#define BAR_PIN_XXX	0x00000010
#DEFINE BAR_PIN_YYY	0x00007C00

in one place and

#define EBI_PINS_PORT0 (FOO_PIN_XXX | FOO_PIN_YYY)
#define EBI_PINS_PORT1 ((BAR_PIN_XXX| BAR_PIN_YYY)

elsewhere, using readable names for the defines.

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

This is exactly what should NOT be necessary to read and understand
the code.

If I see a line like

	rtx->txbd[txIdx].cbd_sc |= (BD_ENET_TX_READY | BD_ENET_TX_LAST |BD_ENET_TX_WRAP);

somewhere in a driver (obviously dealing with ethernet), I don;t have
to look up any specific bits or ping numbers - I can just read it.

If somebody else writes the (computationally equivalent) code

	rtx->txbd[txIdx].cbd_sc |= 0xA800;
or even
	rtx->txbd[txIdx].cbd_sc |= (0x8000 | 0x0800 | 0x2000);

then I don;t have a clue what's going on, and it takes me precious
time to look this up - me and everybody else, each and every time
again when I'm reading the code.

This is simply not acceptable.

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

No, a comment can't fix this, especially as you just have to  wait  a
few months to see the comment and the code getting out of sync.

Please fix the code.

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

Yes, please change it. And patches to fix the existing code that doe
sthis are welcome, too.

> >> +int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> >> +{
> >> +     /* This will reset the CPU core, caches, MMU and all internal buss=
> es */
> >> +     __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.

Famous last words. What if exactrly the PHY is stuck and needs a
reset?

Hmmm... "apart from external memory" ... does  externam  memory  also
include  NOR  flash?  Eventually  the NOR flash you are booting from?
Assume the NOR flash is in query mode when you reset the board -  how
does it get reset, then?


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 impractical for  the  standard  to  attempt  to  constrain  the
behavior  of code that does not obey the constraints of the standard.
                                                          - Doug Gwyn