Dear =?ISO-8859-1?Q?Gunnar_Rang=F8y?=,
In message e1b3d0a60901260726i422cfe85s7c5a35745eedc60f@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