Dear =?ISO-8859-1?Q?Gunnar_Rang=F8y?=,
In message <e1b3d0a60901260726i422cfe85s7c5a35745eedc...@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: w...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
