[U-Boot] [PATCH 01/21] Define new system_restart() and emergency_restart()

[Moffett, Kyle D]

Hi!

On Mar 13, 2011, at 15:24, Wolfgang Denk wrote:
> In message <1299519462-25320-2-git-send-email-Kyle.D.Moffett at boeing.com> you wrote:
>> In preparation for making system restart use a generic set of hooks for
>> boards and architectures, we define some wrappers and weak stubs.
>> 
>> The new wrapper functions are:
>>  system_restart()     -  Normal system reboot (IE: user request)
>>  emergency_restart()  -  Critical error response (IE: panic(), etc)
> 
> What is the difference between these two - and why do we need
> different functions at all?
> 
> A reset is a reset is a reset, isn't it?

That might be true *IF* all boards could actually perform a real hardware reset.

Some can't, and instead they just jump to their reset vector (Nios-II) or to flash (some ppc 74xx/7xx systems).

If the board just panic()ed or got an unhandled trap or exception, then you don't want to do a soft-reset that assumes everything is OK.  A startup in a bad environment like that could corrupt FLASH or worse.  Right now there is no way to tell the difference, but the lower-level arch-specific code really should care.

So system_restart() is what you use when the system is in a good normal operating condition.  The emergency_restart() is what gets called from panic() or in other places where a crash has happened.


> ...
>> +/*
>> + * This MUST be called from normal interrupts-on context.  If it requires
>> + * extended interaction with external hardware it SHOULD react to Ctrl-C.
> 
> ??? Why such complicated restrictions for a simple command that is
> just supposed to reset the board?

This is just documenting what the underlying hardware-specific code is guaranteed.  On some hardware a "system_restart()" may fail and return -1, on others the board needs to cleanly respond to interrupts while polling external hardware, etc.  This is analogous to the normal "sys_reboot()" code under Linux, which requires interrupts-on, etc.

This is to contrast with the emergency_restart() function which has no such API constraints.  It can be called from any context whatsoever and will do its best to either perform a full hardware reset or hang while trying.


>> + * If this function fails to guarantee a clean reboot or receives a Ctrl-C
>> + * keystroke it SHOULD return with an error (-1).
> 
> A "reset" is supposed to take place immediately, and unconditionally.
> If you need delays and ^C handling and other bells and whistles,
> please add these to your own code, but not here.

There's no Ctrl-C handling anywhere in this code, it will all be in my own __board_restart() hook.  As above, this documentation is just describing the guarantees provided to underlying __board_restart() and __arch_restart() hooks; if they check for Ctrl-C while polling external hardware and return an error then that's fine.

>> +int system_restart(void)
>> +{
>> +	int err;
>> +
>> +	/*
>> +	 * Print a nice message and wait a bit to make sure it goes out the
>> +	 * console properly.
>> +	 */
>> +	printf ("Restarting...\n");
>> +	udelay(50000);
>> +
>> +	/* First let the board code try to reboot */
>> +	err = __board_restart();
>> +	if (err)
>> +		goto failed;
>> +
>> +	/* Now call into the architecture-specific code */
>> +	err = __arch_restart();
>> +	if (err)
>> +		goto failed;
>> +
>> +	/* Fallback to the old do_reset() until everything is converted. */
>> +	err = do_reset(NULL, 0, 0, NULL);
>> +
>> +failed:
>> +	printf("*** SYSTEM RESTART FAILED ***\n");
>> +	return err;
>> +}
> 
> You are making a simple thing pretty much complicated.  This adds lots
> of code and I cannot see any benefits.

No, it's actually a net removal of code, the diffstat is:
  90 files changed, 341 insertions(+), 374 deletions(-)

The basic hook structure from system_restart() already existed individually in 6 different architectures (incl. PowerPC, Blackfin, ARM, MIPS...), each of which had its own "board_reset()" or "_machine_restart()" or similar.  This code makes it entirely generic, so as a new board implementor you can simply define a "__board_restart()" function to override (or enhance) the normal architecture-specific code.


> My initial feeling is a plain NAK, for this and the rest of the patch
> series.  Why would we want all this?

While I was going through the hooks I noticed that several of them were explicitly NOT safe if the board was in the middle of a panic() for whatever reason, so I split off the __*_emergency_restart() hooks separately to allow architectures to handle them cleanly.

My own board needs both processor modules to synchronize resets to allow them to come back up at all, which means that a "reset" may block for an arbitrary amount of time waiting for the other module to cleanly shut down and restart (or waiting for somebody to type "reset" on the other U-Boot).  If someone just types "reset" on the console, I want to allow them to hit Ctrl-C to interrupt the process.

Cheers,
Kyle Moffett