Re: [PATCH v8 10/18] RISC-V: Init and Halt Code

[Palmer Dabbelt]

On Wed, 13 Sep 2017 08:20:42 PDT (-0700), Arnd Bergmann wrote:
> On Tue, Sep 12, 2017 at 11:57 PM, Palmer Dabbelt <palmer@xxxxxxxxxxx> wrote:
>> This contains the various __init C functions, the initial assembly
>> kernel entry point, and the code to reset the system.  When a file was
>> init-related this patch contains the entire file.
>
> One minor comment:
>
>> +       /*
>> +        * This hart didn't win the lottery, so we wait for the winning hart to
>> +        * get far enough along the boot process that it should continue.
>> +        */
>> +.Lwait_for_cpu_up:
>> +       REG_L sp, (a1)
>> +       REG_L tp, (a2)
>> +       beqz sp, .Lwait_for_cpu_up
>> +       beqz tp, .Lwait_for_cpu_up
>> +       fence
>
> We usually discourage having the CPUs spin in a busy-loop while
> waiting to be started up, at least on ARM platforms. It seems that
> you could however just have them wait for an interrupt before checking
> the sp/tp values. Would that guarantee to put the CPUs in a low
> power state?

There's no way to _force_ a core to go into a low power state, but putting a
WFI (Wait For Interrupt) in there would at least allow implementations that
care to save power.  We already have some WFIs elsewhere, so it's the right
thing to do here.

There's a few sticky bits here:
* WFI can be implemented as a noop, so this could race with other IPI handling.
  I think it's safe: our IPI is just a "maybe there's a new message" signal, so
  if there's a spurious one it's ignored.
* Interrupts are disabled at this time, though since there's no trap vector it
  should be safe to enable them with a local trap vector here.

I added a FIXME for now, I'll fix it by the v9.

diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
index 1c50fe3765b1..76af908f87c1 100644
--- a/arch/riscv/kernel/head.S
+++ b/arch/riscv/kernel/head.S
@@ -130,6 +130,7 @@ relocate:
         * get far enough along the boot process that it should continue.
         */
 .Lwait_for_cpu_up:
+       /* FIXME: We should WFI to save some energy here. */
        REG_L sp, (a1)
        REG_L tp, (a2)
        beqz sp, .Lwait_for_cpu_up