Hi!
On Mar 13, 2011, at 15:24, Wolfgang Denk wrote:
> In message <1299519462-25320-2-git-send-email-kyle.d.moff...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
