On 21/03/11 23:00, Wolfgang Denk wrote:
Dear Graeme Russ,
In message 4D8739F6.5040805@gmail.com you wrote:
I kind of like the idea of different reset sources (CPU exception, hardware failure, user initiated) but agree copying the linux architecture is over the top.
What's the difference as far as do_reset() is concenred? It shall just (hard) reset the system, nothing else.
Is there any reason reset() could not take a 'reason' parameter? It could be a bit-mask with CPU, SOC and arch reserved bits (unhandled exception, user initiated, panic etc) and board specific bits
What for? To perform the intended purpose, no parameter is needed.
Board or arch specific code could handle different reasons however they please (like logging it in NVRAM prior to restart, gracefully shutting down multiple CPU's, clearing DMA buffers etc)
That would be a layer higher than do_reset() (for example, in panic()).
Hmmm, but panic() is defined in lib/vsprintf.c with no possibility for it to be overridden in any arch or board specific way
All 'hang', 'panic', 'reset' etc code can be simplified into a single code path (although calling 'reset' to 'hang' is a bit odd)
hang() and reset() are intentionally very different things. A call to hang() is supposed to hang (surprise, surprise!) infinitely. It must not cause a reset.
As I said, calling reset() to hang is odd :)
panic() is a higher software layer. It probably results in calling reset() in the end.
Unless CONFIG_PANIC_HANG is defined...
Looking into the code
panic(): - ~130 call sites - Implemented in lib/vsprintf.c - Calls do_reset() if CONFIG_PANIC_HANG is not defined - Calls hang() if CONFIG_PANIC_HANG is defined
hang(): - ~180 call sites using hang() and hang () - Implemented in arch\lib\board.c - ARM, i386, m68k, microblaze, mips, prints "### ERROR ### Please RESET the board ###\n" and loops - avr32 prints nothing and loops - Blackfin can set status LEDs based on #define, prints "### ERROR ### Please RESET the board ###\n" and triggers a breakpoint if a JTAG debugger is connected - nios2 disables interrupts then does the same as ARM et all - powerpc is similar to ARM et al but also calls show_boot_progress(-30) - sh - same as ARM et al but prints "Board ERROR\n" - sparc - if CONFIG_SHOW_BOOT_PROGRESS defined, same as powerpc, else same as ARM et al
So hang() could easily be weakly implemented in common\ which would allow arch and board specific overrides
do_reset(): - ~70 call sites - 39 implemetations - Implemented all over the place (arch/cpu/, arch/lib, board/) - Only ARM is in arch/lib which could likely be moved to arch/cpu - Is U_BOOT_CMD - m68k has a number of very similar/identical implementations - Is not weak - Cannot be overridden at the board level - Ouch, PPC is real ugly: #if !defined(CONFIG_PCIPPC2) && \ !defined(CONFIG_BAB7xx) && \ !defined(CONFIG_ELPPC) && \ !defined(CONFIG_PPMC7XX) /* no generic way to do board reset. simply call soft_reset. */ int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { ... Boards can thus provide their own do_reset() - Wow, m68k/cpu/mcf52x2/cpu.c is even uglier - 7 definitions selectable by #ifdef's - Because do_reset() is U_BOOT_CMD, practically every call is: do_reset (NULL, 0, 0, NULL) - do_bootm() does pass it's args onto do_reset() - No implementation of do_reset() uses any args anyway
reset(): - does not exist (as far as I can tell)
So, maybe we could replace do_reset() with a weak reset() function defined in arch/cpu which can be overridden at the board level. All existing calls to do_reset() can be converted to simply reset()
The U_BOOT_CMD do_reset() would simply call reset()
Could many of the current calls to do_reset() be replaced with calls to panic()?
I still like the idea of passing a 'reason' to reset() / panic() - Could we change panic() to:
void panic(u32 reason, const char *fmt, ...) { va_list args; va_start(args, fmt); vprintf(fmt, args); putc('\n'); va_end(args);
if (reason |= PANIC_FLAG_HANG) hang(reason); else reset(reason); }
Anywhere in the code that needed to hang has a choice - hang(reason) or panic(reason | PANIC_FLAG_HANG)
Default implementations of hang() and reset() would just ignore reason. Board specific code can use reason to do one last boot_progress(), set LED states etc.
Regards,
Graeme