From mboxrd@z Thu Jan  1 00:00:00 1970
From: Graeme Russ <graeme.russ@gmail.com>
Date: Tue, 22 Mar 2011 23:05:46 +1100
Subject: [U-Boot] [PATCH 01/21] Define new system_restart() and
	emergency_restart()
In-Reply-To: <20110321120054.71E63DD9C74@gemini.denx.de>
References: <1299519462-25320-1-git-send-email-Kyle.D.Moffett@boeing.com>
	<1299519462-25320-2-git-send-email-Kyle.D.Moffett@boeing.com>
	<20110313192458.1C19DE0CE97@gemini.denx.de>
	<A60AEA13-1206-4699-9302-0DF9C0F9DE28@boeing.com>
	<20110314185944.56ECC1518DB7@gemini.denx.de>
	<613C8F89-3CE5-4C28-A48E-D5C3E8143A4C@boeing.com>
	<20110314203808.C022B1518DB6@gemini.denx.de>
	<44A75130-ED4F-46D6-B0E4-12433CC15142@boeing.com>
	<20110314220108.683E51518DB6@gemini.denx.de>
	<4D8739F6.5040805@gmail.com>
	<20110321120054.71E63DD9C74@gemini.denx.de>
Message-ID: <4D88909A.80508@gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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